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

Allow us to specify the path to post telemetry to in the JaegerHttpClient #2821

Closed
abe545 opened this issue Jan 27, 2022 · 2 comments · Fixed by #2847
Closed

Allow us to specify the path to post telemetry to in the JaegerHttpClient #2821

abe545 opened this issue Jan 27, 2022 · 2 comments · Fixed by #2847
Labels
enhancement New feature or request

Comments

@abe545
Copy link
Contributor

abe545 commented Jan 27, 2022

Feature Request

Is your feature request related to a problem?

In the JaegerHttpClient, the path to post is hard-coded to /api/traces: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerHttpClient.cs#L70
I'd like to be able to specify this because the way our infrastructure is setup is that the listener is expecting a different path. And it won't collect trace data if that is appended. We're switching from the Jaeger library with OpenTracing, and it worked previously. I see that the zipkin exporter allows you to specify the endpoint directly

Describe the solution you'd like:

I'd either like to be able to do the same, or have this hard-coded path be configurable.

Describe alternatives you've considered.

I can use the zipkin exporter, but since we've been using Jaeger already, I'd like to stick with it (plus all the other non .net services still use Jaeger).

Additional Context

I'm happy to implement the feature myself, but thought I should open it up for discussion before doing so.

@abe545 abe545 added the enhancement New feature or request label Jan 27, 2022
@mic-max
Copy link
Contributor

mic-max commented Feb 2, 2022

I think this would make a fine addition @abe545!
Likely will need to add another parameter to the JaegerHttpClient constructor (or a new constructor, or default value parameter) and a field on the class. If omitted, use the existing /api/traces value.
I'll let you propose the PR and make these implementation choices though 😅

@abe545
Copy link
Contributor Author

abe545 commented Feb 2, 2022

Thanks @mic-max! I was on the fence about adding a new option to the options class, or if I should just use the Endpoint that exists on the options - if it doesn't include a path or query, to use the /api/traces. But if it does include one, to just use it as is. The latter might be slightly confusing, so I'll probably add a new option.

One thing that I was also considering is that this behaves differently from the zipkin exporter. That one just takes the configured endpoint as-is. It would be nice for them to have consistent behavior, but just removing the /api/traces from this class seemed like it would cause a lot of headaches for everybody that currently has this working correctly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants