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

Support for Jaeger Propagator #1881

Closed
kradalby opened this issue Mar 8, 2021 · 18 comments · Fixed by #3309
Closed

Support for Jaeger Propagator #1881

kradalby opened this issue Mar 8, 2021 · 18 comments · Fixed by #3309
Assignees
Labels
enhancement New feature or request help wanted Good for taking. Extra help will be provided by maintainers

Comments

@kradalby
Copy link

kradalby commented Mar 8, 2021

Feature Request

The OpenTelemetry specification suggests that a propagator for Jaeger MUST be implemented alongside the B3 (Zipkin) and W3C Trace Context.

I cannot seem to find any traces of such a Propagator being implemented and it would be great if it was :) Some third party applications like Nginx, used in many proxies rely on this format and valuable trace information can be lost in that step.

Is your feature request related to a problem?

Some application, typically third party, rely on "older" Opentracing implementation using Jaeger tracing context and this information is not being tied together with the current implementation of the dotnet library.

Describe the solution you'd like:

Adding a propagator that implements, understands and propagate the Jaeger (uber header) format.

If this has been implemented and I cannot find it, I apologies, I just cannot seem to find any information about it.

Thanks
Kristoffer

@kradalby kradalby added the enhancement New feature or request label Mar 8, 2021
@cortex93
Copy link

Having the same needs, I have done a quick and dirty extractor

https://gist.github.com/cortex93/a910534ba8bd095428102a30d0ba4eb9
The getter to extract header expects a fixed name while baggage item are prefixed header. I don't know how to extract them.

Injecting in TraceContext format should be compatible with legacy Jaeger client. Thus, I don't add an inject implementation.

In you startup code,

Sdk.SetDefaultTextMapPropagator(new CompositeTextMapPropagator(new TextMapPropagator[]
            {
                new TraceContextPropagator(),
                new JaegerPropagator()
            }));
curl -H "uber-trace-id: 80f198ee56343ba864fe8b2a57d3eff7:e457b5a2e4d86bd1:05e3ac9a4f6e3b90:1" localhost:5000

with consoleexporter, it will print out

Activity.Id:          00-80f198ee56343ba864fe8b2a57d3eff7-47c58c6d3ea0484b-01
Activity.ParentId:    00-80f198ee56343ba864fe8b2a57d3eff7-e457b5a2e4d86bd1-00

Waiting for a proper implementation and a way to support baggage header.

@cijothomas cijothomas added the help wanted Good for taking. Extra help will be provided by maintainers label Mar 20, 2021
@SVronskiy
Copy link

Hello!

Same here. Using Linkerd assumes we are using B3 headers propagation among services. How to implement this?

Thanks in advance.

@notcool11
Copy link
Contributor

notcool11 commented Mar 11, 2022

i'll be working on adding a jaeger propagator. @cijothomas , can you assign this to me?? thanks!!

@cijothomas
Copy link
Member

i'll be working on adding a jaeger propagator. @cijothomas , can you assign this to me?? thanks!!

done. It must be a new package from this repo. (The fact b3 is part of the API package is a bug).

@dmpe
Copy link
Contributor

dmpe commented Apr 8, 2022

@cijothomas could you maybe clarify what you mean by that? Would the other C# repository https://github.com/open-telemetry/opentelemetry-dotnet-contrib be appropriate for it? Looking at https://github.com/Progressive-Insurance/opentelemetry-dotnet/pull/1/files changes it should be possible, or am i wrong? Thanks.

@notcool11
Copy link
Contributor

uh oh, i missed that comment @cijothomas . darn. been fighting with my company on the CLA stuff and just got is squared away and now it will have to be a new repo?? thanks for bringing this to attention @dmpe .

@notcool11
Copy link
Contributor

Looking at the link from the OP, this seems a bit up for interpretation:

The official list of propagators that MUST be maintained by the OpenTelemetry organization and MUST be distributed as OpenTelemetry extension packages.

@cijothomas , are you saying that only W3C should be native to the dotnet SDK and B3/Jaeger are classified as extensions??

@cijothomas
Copy link
Member

are you saying that only W3C should be native to the dotnet SDK and B3/Jaeger are classified as extensions??

Yes. A new package.
(its a mistake that B3 was made part of the main API package..)

@notcool11
Copy link
Contributor

@cijothomas , well that's a bummer. Okay, so if it has to go in the contrib repo does this issue need to move over there also or do I just open a feature request over there??

@cijothomas
Copy link
Member

@notcool11 A new package, (like OpenTelemetry.Propagator.Jaeger), but it can be in this repo. No need to move to -contrib repo.

@notcool11
Copy link
Contributor

@cijothomas , can you point me to some documentation on getting that set up?? Or is it as simple as just creating a new project in the solution and there is some voodoo magic that automatically does the packaging?? thanks.

@cijothomas
Copy link
Member

is it as simple as just creating a new project in the solution

yes. packaging is done with MinVer tool. we can decide the versioning later, when we are ready for the 1st release.

@notcool11
Copy link
Contributor

@cijothomas , ran into the first snag.

Created the new project but there are several items in the OpenTelemetry.Internal namespace that was accessible in the OpenTelemetry.Api project that is not accessible now.

Okay to add my new project to the internals visible to?? Specifically I was using OpenTelemetryApiEventSource for the various propagation events. Thanks.

@cijothomas
Copy link
Member

@cijothomas , ran into the first snag.

Created the new project but there are several items in the OpenTelemetry.Internal namespace that was accessible in the OpenTelemetry.Api project that is not accessible now.

Okay to add my new project to the internals visible to?? Specifically I was using OpenTelemetryApiEventSource for the various propagation events. Thanks.

To unblock, and temporarily, yes use InternalsVisible. After the initial round of PRs, you can modify to use own eventsource, and remove the internalsvisible.

@notcool11
Copy link
Contributor

@cijothomas , looks like other projects have handled it by cherry picking specific files out of the internal folder.

<ItemGroup> <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\ExceptionExtensions.cs" Link="Includes\ExceptionExtensions.cs" /> <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\SemanticConventions.cs" Link="Includes\SemanticConventions.cs" /> <Compile Include="$(RepoRoot)\src\OpenTelemetry.Api\Internal\SpanAttributeConstants.cs" Link="Includes\SpanAttributeConstants.cs" /> </ItemGroup>

Is that preferred??

@dmpe
Copy link
Contributor

dmpe commented May 14, 2022

@notcool11 I have recently started to move B3 propagators to the new package Extensions.Propagators as well. After #3271 will be merged 💛 , deprecating B3 in the OpenTelemetry.API package would be a remaining todo.

I am not sure if you saw it; just wanted to let you know that package is already there :) and could be used. 👍

@notcool11
Copy link
Contributor

hahaha, change it again!! thanks for the heads up @dmpe.

@cijothomas, what would you like to do?? i just refreshed my fork from main and got the new OpenTelemetry.Propagator.Jaeger projects building/testing. Do you want me to wait and integrate with #3271 ??

@notcool11
Copy link
Contributor

notcool11 commented May 20, 2022

@cijothomas, looks like #3271 got merged in and the zipkin propagator is in it. I'm gonna change again and add the Jaeger one there.

@dmpe , might reach out to you if i have any issues. thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Good for taking. Extra help will be provided by maintainers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants