Allow the jaeger exporter path to be configured #2847

abe545 · 2022-02-02T20:42:57Z

Changes

Please provide a brief description of the changes here.
This makes the JaegerExporter treat the configurable Endpoint in its options the same way the ZipkinExporter does. Basically, instead of hardcoding /api/traces as a path to POST the trace data to, it is now assumed to be a part of the Endpoint. This is the cleanest way of implementing this feature, but it is possible it will break people who already have it configured correctly.

I'm open to other ways of doing this - I'm not sure what the ethos is in this project surrounding changes like this. As I mentioned in the original feature request, I have some other ideas about how to achieve the goal (the goal being to be able to fully control the URL being used to post trace data), but figured I'd start with the cleanest code and move from there.

For significant contributions please make sure you have completed the following items:

Appropriate CHANGELOG.md updated for non-trivial changes
Design discussion issue # (Clarify how the default value of OTEL_EXPORTER_JAEGER_ENDPOINT should work opentelemetry-specification#2331)
Changes in public API reviewed

linux-foundation-easycla · 2022-02-02T20:43:00Z

The committers are authorized under a signed CLA.

✅ Abraham Heidebrecht (6e0ce44, 630424c, 141df78, 9701f99)

test/OpenTelemetry.Exporter.Jaeger.Tests/OpenTelemetry.Exporter.Jaeger.Tests.csproj

cijothomas · 2022-02-02T21:04:34Z

The committers are authorized under a signed CLA.

✅ Abraham Heidebrecht (6e0ce44, 630424c, 141df78, 9701f99, e007798)

abe545 · 2022-02-02T21:05:59Z

The committers are authorized under a signed CLA.

✅ Abraham Heidebrecht (6e0ce44, 630424c, 141df78, 9701f99, e007798, 9a2eac7)

test/OpenTelemetry.Exporter.Jaeger.Tests/OpenTelemetry.Exporter.Jaeger.Tests.csproj

reyang

LGTM with minor suggestions.

cijothomas · 2022-02-03T21:56:02Z

src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs

        /// </summary>
-        public Uri Endpoint { get; set; } = new Uri("http://localhost:14268");
+        public Uri Endpoint { get; set; } = new Uri(DefaultJaegerEndpoint);


I think this is a breaking behavior. Previously /api/traces was appended to the endpoint user provided. Now we expect the user to provide the whole url.

(should be fine as Endpoint was added only in 1.2 and we haven;t yet released a stable)

@CodeBlanch Could you comment?

Also, we should update the readme to reflect the new change.

Agreed that this a breaking change. As I said in my PR comment, I personally think it makes sense to have this work like the zipkin exporter does - but I totally understand if you would like to avoid a breaking change. This is fairly trivial to change to some other pattern (like maybe another property on the JaegerExporterOptions with the path to append to the endpoint, and have it default to /api/traces).
I'll update the readme as a part of this PR to reflect the decision you make on how to implement this.

Hi @cijothomas just wanted to check if you've had a chance to discuss with the other maintainers what you'd like me to do here? Thanks!

It may be good to confront it with the specification and how other languages are doing it (I suspect inconsistency after a quick look).

PS. OTel Java docs are probably misleading.

So in the Environment variable docs it just lists http://localhost:14250 as the default value, and the jaeger docs are explicit about the path to use for Thrift over HTTP: /api/traces. I guess where this breaks down for my organization is that we don't use the jaeger agent, but rather a third party one that fails when we use this combination of values.

I've done some investigation on other otel languages:

Java - it looks like it only supports grpc

Javascript - Namely is already successfully sending metrics to our agent (It looks like otel js is using the jaeger-client npm package, which just uses the endpoint as-is):

const exporter = new JaegerExporter({ endpoint: 'http://our-trace-collector:9080/v1/trace', });

Golang - according to their readme and docs, the endpoint can be specified with a path (OTEL_EXPORTER_JAEGER_ENDPOINT defaults to http://localhost:14268/api/traces), and looking through the code, it just uses the endpoint value specified

Ruby - readme and code indicate that they export to the URL specified in the env var as-is

I haven't looked through any of the other languages, but it does seem that my change is more consistent with at least a few of the other languages that otel supports (and incidentally all the ones my Company uses 😄).

FYI open-telemetry/opentelemetry-go#2599

One other thing I forgot to mention is that the behavior in this repo is different from the OpenTracing jaeger client (which we're migrating from). It also just used the endpoint property/env-var as-is.
If you want to keep the behavior as-is, would you have a problem with me adding a new configuration option to set the path? Right now there is no way for me to use jaeger with open telemetry in my organization for .net. That is the problem I'm trying to solve, and I'm not terribly concerned about how to solve it.

Anyway, I opened an issue in the otel spec repo: open-telemetry/opentelemetry-specification#2331

Now that it looks like the spec will be updated to make it clear that the endpoint should include the path, how do you feel about this pr @pellared and @cijothomas (also, do I need to do something to kick off the build)?

@abe545 In general I prefer the approach of your PR (this is also how I implemented it in OTel Go SDK some time ago 😄). The change is not really breaking from the perspective as there was no stable release.

cijothomas

Might be breaking, so want to make sure we are okay with this.
https://github.com/open-telemetry/opentelemetry-dotnet/pull/2847/files#r799001655

codecov · 2022-02-11T10:07:50Z

Codecov Report

Merging #2847 (1f7ec6a) into main (1dec9bd) will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2847      +/-   ##
==========================================
+ Coverage   83.86%   84.01%   +0.15%     
==========================================
  Files         254      254              
  Lines        8943     8942       -1     
==========================================
+ Hits         7500     7513      +13     
+ Misses       1443     1429      -14

Impacted Files	Coverage Δ
...Exporter.Jaeger/Implementation/JaegerHttpClient.cs	`91.66% <100.00%> (+47.66%)`	⬆️
...Telemetry.Exporter.Jaeger/JaegerExporterOptions.cs	`100.00% <100.00%> (ø)`
...nTelemetry.Exporter.Jaeger/Implementation/Batch.cs	`100.00% <0.00%> (+5.55%)`	⬆️

github-actions · 2022-02-23T03:17:22Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

abe545 · 2022-02-23T16:08:06Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@cijothomas Is there any chance you can take a look at this pr again before github closes it?

cijothomas · 2022-02-24T18:13:23Z

@abe545 The change looks good to me. Please address the following before merge:

Please update changelog to include this. (highlight that this will break previous rc users, so they need to take action).
Update PR Description to include the relevant Spec links.

dismissing old review.

CodeBlanch

LGTM

abe545 · 2022-02-24T18:45:39Z

@abe545 The change looks good to me. Please address the following before merge:

Please update changelog to include this. (highlight that this will break previous rc users, so they need to take action).

Update PR Description to include the relevant Spec links.

Thanks @cijothomas! I updated the changelog and PR comment - please let me know if you'd like me to make any other changes

src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md

cijothomas · 2022-02-24T22:21:29Z

@abe545 Could you merge the latest main to this as well?

Allow the jaeger exporter path to be configured

6e0ce44

abe545 requested a review from a team February 2, 2022 20:42

cijothomas reviewed Feb 2, 2022

View reviewed changes

test/OpenTelemetry.Exporter.Jaeger.Tests/OpenTelemetry.Exporter.Jaeger.Tests.csproj Outdated Show resolved Hide resolved

Do not pull in extra dependencies

630424c

reyang reviewed Feb 2, 2022

View reviewed changes

test/OpenTelemetry.Exporter.Jaeger.Tests/OpenTelemetry.Exporter.Jaeger.Tests.csproj Outdated Show resolved Hide resolved

reyang approved these changes Feb 2, 2022

View reviewed changes

Abraham Heidebrecht added 2 commits February 2, 2022 17:00

Move the link in the csproj to the same spot as the rest

141df78

Added assertion for the Endoint in the JaegerExporterOptionsTests

9701f99

reyang approved these changes Feb 2, 2022

View reviewed changes

mic-max approved these changes Feb 3, 2022

View reviewed changes

cijothomas reviewed Feb 3, 2022

View reviewed changes

cijothomas previously requested changes Feb 3, 2022

View reviewed changes

abe545 mentioned this pull request Feb 10, 2022

Clarify how the default value of OTEL_EXPORTER_JAEGER_ENDPOINT should work open-telemetry/opentelemetry-specification#2331

Closed

Merge branch 'main' into add-configurable-path-to-jaeger-exporter

e007798

pellared approved these changes Feb 11, 2022

View reviewed changes

abe545 requested a review from cijothomas February 15, 2022 17:31

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Feb 23, 2022

Merge branch 'main' into add-configurable-path-to-jaeger-exporter

9a2eac7

github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Feb 24, 2022

cijothomas added this to the 1.2.0 milestone Feb 24, 2022

CodeBlanch approved these changes Feb 24, 2022

View reviewed changes

Added change to the changelog

c12b879

cijothomas reviewed Feb 24, 2022

View reviewed changes

src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md Outdated Show resolved Hide resolved

Abraham Heidebrecht added 2 commits February 24, 2022 17:17

Constrain the width to 80 characters to please the linter

eb3c20b

Include links in the changelog

102edb0

Merge branch 'main' into add-configurable-path-to-jaeger-exporter

1f7ec6a

cijothomas approved these changes Feb 24, 2022

View reviewed changes

cijothomas merged commit 13e721d into open-telemetry:main Feb 24, 2022

abe545 deleted the add-configurable-path-to-jaeger-exporter branch February 24, 2022 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow the jaeger exporter path to be configured #2847

Allow the jaeger exporter path to be configured #2847

abe545 commented Feb 2, 2022 •

edited

Loading

linux-foundation-easycla bot commented Feb 2, 2022 •

edited

Loading

cijothomas commented Feb 2, 2022 •

edited by linux-foundation-easycla bot

Loading

abe545 commented Feb 2, 2022 •

edited by linux-foundation-easycla bot

Loading

reyang left a comment

cijothomas Feb 3, 2022

abe545 Feb 3, 2022

abe545 Feb 8, 2022

pellared Feb 9, 2022 •

edited

Loading

abe545 Feb 9, 2022 •

edited

Loading

pellared Feb 10, 2022

abe545 Feb 10, 2022

abe545 Feb 10, 2022

abe545 Feb 10, 2022

pellared Feb 11, 2022

cijothomas left a comment •

edited

Loading

codecov bot commented Feb 11, 2022 •

edited

Loading

github-actions bot commented Feb 23, 2022

abe545 commented Feb 23, 2022

cijothomas commented Feb 24, 2022

CodeBlanch left a comment

abe545 commented Feb 24, 2022

cijothomas commented Feb 24, 2022

Allow the jaeger exporter path to be configured #2847

Allow the jaeger exporter path to be configured #2847

Conversation

abe545 commented Feb 2, 2022 • edited Loading

Changes

linux-foundation-easycla bot commented Feb 2, 2022 • edited Loading

cijothomas commented Feb 2, 2022 • edited by linux-foundation-easycla bot Loading

abe545 commented Feb 2, 2022 • edited by linux-foundation-easycla bot Loading

reyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

abe545 Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Feb 11, 2022 • edited Loading

Codecov Report

github-actions bot commented Feb 23, 2022

abe545 commented Feb 23, 2022

cijothomas commented Feb 24, 2022

CodeBlanch left a comment

Choose a reason for hiding this comment

abe545 commented Feb 24, 2022

cijothomas commented Feb 24, 2022

abe545 commented Feb 2, 2022 •

edited

Loading

linux-foundation-easycla bot commented Feb 2, 2022 •

edited

Loading

cijothomas commented Feb 2, 2022 •

edited by linux-foundation-easycla bot

Loading

abe545 commented Feb 2, 2022 •

edited by linux-foundation-easycla bot

Loading

pellared Feb 9, 2022 •

edited

Loading

abe545 Feb 9, 2022 •

edited

Loading

cijothomas left a comment •

edited

Loading

codecov bot commented Feb 11, 2022 •

edited

Loading