Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't re-call SetGlobalTracer with two different types of tracer #198

Closed
iredelmeier opened this issue Oct 12, 2019 · 9 comments
Closed
Milestone

Comments

@iredelmeier
Copy link
Member

This seems to be due to the usage of atomic.Value, which expects to be used with a "consistently typed value".

Running this code:

package main

import (
	"go.opentelemetry.io/api/trace"
	sdktrace "go.opentelemetry.io/sdk/trace"
)

func main() {
	trace.SetGlobalTracer(trace.NoopTracer{})
	sdktrace.Register() // calls trace.SetGlobalTracer()
}

triggers a panic from sync/atomic.

@jmacd
Copy link
Contributor

jmacd commented Oct 12, 2019

OTEP 0005 says you can only initialize the global once. If you're testing, please avoid the global instance.

@iredelmeier
Copy link
Member Author

Hmm... Avoiding the global instance completely in testing feels unsustainable. Some examples:

  • Testing that plugins use the global if one is not explicitly provided
  • Application tests that use a mix of test and real (e.g., SDK) tracers depending on what level they're testing

@jmacd
Copy link
Contributor

jmacd commented Oct 14, 2019

I do not believe plugins should use the global tracer.
In fact I think any code that is tested should not use the global tracer.
But if you still believe that tests should be able to replace the tracer instance, my best recommendation is to create a new kind of Tracer instance that can be installed once, that lets you swap in new exporters dynamically, or something like that. I can't imagine a test that wants both real and test-oriented tracers.

Restricting the global to be set only once is a major step in alleviating the concerns of people who dislike globals. It prevents the potential for uncoordinated code to overwrite the global tracer at startup, which could lead to chaos.

@iredelmeier
Copy link
Member Author

@jmacd what do you mean that plugins should not use the global tracer? If anything, I've received pushback against the idea that plugins should permit users to not use global tracers!

We also already use the global tracer in tests, both explicitly (e.g., here and implicitly (e.g., by using the SDK, which can only use the global tracer right now since that's the only publicly exposed mechanism).

For example, adding:

func TestFoo(t *testing.T) {
	trace.SetGlobalTracer(trace.NoopTracer{})
}

anywhere in the propagation_test or the sdk/trace packages will cause all of those tests to fail due to their existing reliance on the global tracer.

jmacd pushed a commit to jmacd/oteps that referenced this issue Oct 14, 2019
@jmacd
Copy link
Contributor

jmacd commented Oct 14, 2019

I updated open-telemetry/oteps#45

I expect that plugins would be configured explicitly and installed in the process somehow, and that the application main() function would be the responsible party. I see plugins as different from third-party libraries, which are the intended beneficiaries of the global SDK. I suppose there will be configuration-free plugins that we want to install as an import statement, in which case I propose the use of a test-specific SDK that lets you install exporters dynamically for testing purposes.

@jmacd
Copy link
Contributor

jmacd commented Oct 14, 2019

(Even for configuration-free libraries, I would prefer to see the main() function both import the module and call some kind of Init() method on the plugin, passing in the Tracer and/or Meter.)

@iredelmeier
Copy link
Member Author

I updated open-telemetry/oteps#45

I expect that plugins would be configured explicitly and installed in the process somehow, and that the application main() function would be the responsible party. I see plugins as different from third-party libraries, which are the intended beneficiaries of the global SDK. I suppose there will be configuration-free plugins that we want to install as an import statement, in which case I propose the use of a test-specific SDK that lets you install exporters dynamically for testing purposes.

I think the (or at least my!) usage of "plugin" might be part of the confusion here 😅

To clarify, I'm referring to both first- and third-party code.

@hodgesds
Copy link

This makes the SDK Tracer practically useless for extending, because the only way to get an instance of it is through the Register function which sets the global.

@rghetia
Copy link
Contributor

rghetia commented Oct 24, 2019

Global tracer registry is replaced with global trace provider. This was done in #227. This issue was fixed for provider registry.

@rghetia rghetia closed this as completed Oct 24, 2019
hstan referenced this issue in hstan/opentelemetry-go Oct 15, 2020
…aws (#198)

* Bump github.com/aws/aws-sdk-go from 1.33.20 to 1.33.21 in /detectors/aws

Bumps [github.com/aws/aws-sdk-go](https://github.com/aws/aws-sdk-go) from 1.33.20 to 1.33.21.
- [Release notes](https://github.com/aws/aws-sdk-go/releases)
- [Changelog](https://github.com/aws/aws-sdk-go/blob/master/CHANGELOG.md)
- [Commits](aws/aws-sdk-go@v1.33.20...v1.33.21)

Signed-off-by: dependabot[bot] <[email protected]>

* Auto-fix go.sum changes in dependent modules

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com>
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants