search

jaegertracing / jaeger-client-go

🛑 This library is DEPRECATED!
https://jaegertracing.io/
Apache License 2.0
1.39k stars 288 forks source link

128 bit mode should be default #604

Closed wbl closed 2 years ago

wbl commented 2 years ago

Describe the bug 128 bit mode is not default

To Reproduce

  1. Follow the examples in a high traffic service
  2. Get wrecked with duplicate span IDs

Expected behavior Not have duplicate traces. By default things should work, not break. Having a "set this or have a problem" flag is bad.

Additional context While it's now possible to specify 128 bit span ID generation, it requires a different way to initialize from explicit transport initialization, and the documentation on how to translate from one to the other is not very clear.

yurishkuro commented 2 years ago

Changing the default would be a breaking change, requiring a major release, which we're not planning to do since the library is being retired (see README).

The 128bit mode can be set via env var JAEGER_TRACEID_128BIT or programmatically via config. Feel free to suggest improvements to the docs on how to make this more clear.

wbl commented 2 years ago

How would it be a breaking change? Are there receivers it would break? It certainly wouldn't break the compiles.

As for configuration, we weren't using that, but making the tracer directly and then installing it as the global tracer. At this point there are ~two different ways to configure a tracer, and they don't seem to be equivalent. Clearly indicating which is depreciated and provide equivalents for each setting in the one that is for the one that is not would be helpful.

With depreciation will there be a migration guide?

yurishkuro commented 2 years ago

How would it be a breaking change?

There are existing deployments that rely on the default to be 64bit. Changing that default may break those deployments.

there are ~two different ways to configure a tracer, and they don't seem to be equivalent.

if you're talking about programmatic configuration, please provide specific references to code where you see the duplication.

wbl commented 2 years ago

https://pkg.go.dev/github.com/uber/jaeger-client-go#NewTracer and https://pkg.go.dev/github.com/uber/jaeger-client-go@v2.29.1+incompatible/config#Configuration.New both create tracers. What's not obvious is the list of available TracerOptions in the documentation, because they are public methods on a private type (but public variable). That's what threw me for a loop.

yurishkuro commented 2 years ago

Right. So the Configuration type is meant for declarative tracer initialization, e.g. you could populate that struct from a YAML/JSON configuration. The NewTracer() constructor is lower-level initialization that gives you more control over which components to instantiate (although Config has some of that too, for convenience).

I agree, the functional options on private type were unfortunate choice - that could be improved as a non-breaking change, if you wish to propose a PR.