Added Jaeger Propagator to Opentelemetry.Extensions.Propagators #3309

notcool11 · 2022-05-26T16:11:10Z

added jaeger progagator with tests
updated changelog
moved comment to unreleased

Fixes #1881 .

Changes

Added Jaeger Propagator to the new Opentelemetry.Extensions.Propagators project with tests. Been running this code locally as a standalone propagator in production for about six months.

Couple of notes, I've found that URL encoding in headers is not consistent so sometimes the uber-trace-id is delimited with a : colon and sometimes encoded to %3A. There is also code to handle the shorter 64 traceid vs. the newer 128 bit format.

Unit tests should cover all these items. Thanks.

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

Appropriate CHANGELOG.md updated for non-trivial changes
Design discussion issue #
Changes in public API reviewed

* added jaeger progagator with tests * updated changelog * moved comment to unreleased

linux-foundation-easycla · 2022-05-26T16:11:13Z

The committers listed above are authorized under a signed CLA.

✅ login: notcool11 / name: tyler jago (9c19f44)

reyang · 2022-05-26T16:54:34Z

More of a question for the maintainers - do we want different propagators to be bundled as a single package or they should be decoupled?

notcool11 · 2022-05-26T17:06:11Z

More of a question for the maintainers - do we want different propagators to be bundled as a single package or they should be decoupled?

Oh please no. :(

The discussion in 1881 has bounced several times already. I had it separate before 3244 was merged in and created the new extensions package.

cijothomas · 2022-05-26T17:21:22Z

More of a question for the maintainers - do we want different propagators to be bundled as a single package or they should be decoupled?

The Non W3C ones, zipkin/jaeger can be a single package. I think @alanwest suggested it that way earlier, and I agree.
The W3C is part of the main package itself.

reyang

LGTM.

codecov · 2022-05-26T17:54:19Z

Codecov Report

Merging #3309 (91415d0) into main (efc55d5) will decrease coverage by 0.11%.
The diff coverage is 95.38%.

@@            Coverage Diff             @@
##             main    #3309      +/-   ##
==========================================
- Coverage   85.90%   85.78%   -0.12%     
==========================================
  Files         267      268       +1     
  Lines        9383     9448      +65     
==========================================
+ Hits         8060     8105      +45     
- Misses       1323     1343      +20

Impacted Files	Coverage Δ
...lemetry.Extensions.Propagators/JaegerPropagator.cs	`95.38% <95.38%> (ø)`
...entation/ExportClient/OtlpGrpcTraceExportClient.cs	`50.00% <0.00%> (-28.58%)`	⬇️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs	`59.09% <0.00%> (-18.19%)`	⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	`76.47% <0.00%> (-5.89%)`	⬇️
...metryProtocol/Implementation/ActivityExtensions.cs	`91.20% <0.00%> (-3.30%)`	⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs	`97.14% <0.00%> (-2.86%)`	⬇️

src/OpenTelemetry.Extensions.Propagators/CHANGELOG.md

src/OpenTelemetry.Extensions.Propagators/JaegerPropagator.cs

github-actions · 2022-06-03T03:31:42Z

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.

nit from PR comments

notcool11 · 2022-06-07T16:16:12Z

@reyang , @cijothomas , @alanwest
this was a pet project for me at my current company, but my last day here will be 06/10. i'll not have access to the fork after i leave. i also don't have confidence that anyone else at the company will care enough to try to babysit this PR to make any changes.

pellared · 2022-06-08T06:20:52Z

@notcool11, are you able to fix the build failures?

notcool11 · 2022-06-08T12:57:26Z

@pellared , yep. was failing after pulling latest, had to update the event source per @cijothomas . building in my fork now, should be done shortly

* Refactor string extensions (#3306) * fixing InMemoryExporter & Metrics bug. new class: MetricSnapshot. new ctor: InMemoryExporter(Func) (#3266) * Exporting tags consistently (#3281) * feat(tracing): deprecate use of B3 propagator class from API package - use Extensions.Propagators package instead (#3289) * Use TagTransformer for ConsoleExporter 💻 (#3311) * Add core tag prefix to OTLP logs project (#3316) * Add Extensions.Propagators to core (#3318) * Changelog update for 1.3.0-rc.1 release prep (#3320) * Adjust changelog to reflect actual date and package (#3327) * Update protos to 0.18.0 (#3321) Co-authored-by: Cijo Thomas <[email protected]> * Propagation in case of remote parent (#3310) * Replace MOQ with InMemoryExporter in ASP.NET Core (#3328) * cleanup CI (#3332) * ConsoleMetricExporter to export all resource attributes (#3334) * Fix EventSource for B3Propagator (#3336) * Update changelog to prepare release (#3337) * Move public api to shipped (#3338) * Post release cleanups (#3340) * Fix NullReferenceException caught by SDK when metric has a tag with a null value (#3325) * Add isRemote check for context propagation (#3329) * Minor test fixes sln additions (#3341) * Update .sln file;Remove docker compose file for net5.0 (#3347) * Remove StackExchangeRedis Instrumenation (#3346) Co-authored-by: Cijo Thomas <[email protected]> * fixed event source in jaegerpropagator after pulling latest Co-authored-by: Piotr Kiełkowicz <[email protected]> Co-authored-by: Timothy Mothra <[email protected]> Co-authored-by: Alan West <[email protected]> Co-authored-by: John <[email protected]> Co-authored-by: Cijo Thomas <[email protected]> Co-authored-by: Vishwesh Bankwar <[email protected]> Co-authored-by: Reiley Yang <[email protected]> Co-authored-by: Utkarsh Umesan Pillai <[email protected]>

notcool11 · 2022-06-08T14:29:45Z

@pellared , yep. was failing after pulling latest, had to update the event source per @cijothomas . building in my fork now, should be done shortly

Green in my fork now.

pellared · 2022-06-08T14:32:24Z

There are conflicts with main

notcool11 · 2022-06-08T14:51:49Z

@pellared , ugh, that's lame. i didn't even change that file. stupid refresh from main now shows 129 files changed when really i just have 5. what a pita.

notcool11 · 2022-06-08T14:53:35Z

is there a better way to refresh my fork than this??
git pull https://github.com/open-telemetry/opentelemetry-dotnet.git

pellared · 2022-06-08T14:57:20Z

git remote add upstream https://github.com/open-telemetry/opentelemetry-dotnet.git
git fetch -p upstream
git merge upstream/main # or git rebase upstream/master - but this would require force push

notcool11 · 2022-06-08T15:00:32Z

only files that matter in this PR:
src/OpenTelemetry.Extensions.Propagators/.publicApi/net462/PublicAPI.Unshipped.txt
src/OpenTelemetry.Extensions.Propagators/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt
src/OpenTelemetry.Extensions.Propagators/CHANGELOG.md
src/OpenTelemetry.Extensions.Propagators/JaegerPropagator.cs
test/OpenTelemetry.Extensions.Propagators.Tests/JaegerPropagatorTest.cs

do you need me to pull from main again or can you just merge and ignore changes in this PR except the above files?? my guess is this fork will continue to drift and will need to be rebased.

notcool11 · 2022-06-08T15:01:11Z

git remote add upstream https://github.com/open-telemetry/opentelemetry-dotnet.git
git fetch -p upstream
git merge upstream/main # or git rebase upstream/master - but this would require force push

okay, i'll try that

src/OpenTelemetry.Extensions.Propagators/CHANGELOG.md

Co-authored-by: Robert Pająk <[email protected]>

* Added Jaeger Propagator to Opentelemetry.Extensions.Propagators (#3309) * Remove unnecessary bullet in CHANGELOG.md (#3352) Co-authored-by: Cijo Thomas <[email protected]> * Fix OTLP test (#3357) * Show that test is not doing what you might think it does * More asserts the merrier * Show this little test that it has potential * improve test coverage: InMemoryExporter & IDeferredMeterProviderBuilder (#3345) * [SDK] Circular buffer tweaks + cpu pressure test (#3349) * CircularBuffer tweaks and cpu pressure test. * Switch to Volatile.Read. * Perf tweaks. * Remove race check in debug after doing more testing. Co-authored-by: Cijo Thomas <[email protected]> * Fix event name logic + support null categoryname. (#3359) * In-memory Exporter: Buffer log scopes (#3360) * Buffer log scopes when using in-memory exporter. * CHANGELOG update. * Code review. * Tests. * CHANGELOG tweak. * SDK: Forward SetParentProvider to children of CompositeProcessor (#3368) * Examples: Fix ParentProvider not being set on MyFilteringProcessor (#3370) * Fix ParentProvider not being set on MyFilteringProcessor example. * Added XML comments. * Tweak. * Typo. * Logs: Add helper ctors & forceflush on OpenTelemetryLoggerProvider (#3364) * Add helper ctors & forceflush on OpenTelemetryLoggerProvider. * CHANGELOG update. * Unit tests. * Code review. * Code review. * Tweak. * SDK: Nullable annotations for base classes & batch + shims to enable compiler features (#3374) * Nullable annotations and shims for SDK base classes & batch. * Target updates. * Remove System.Collections.Immutable ref. * ApiCompat attribute exclusions. * ASPNETCore instrumentation to populate httpflavor tag (#3372) * improve test coverage: InMemoryExporter SnapshotMetric (#3344) * Fix AspNetCore metrics to use correct value for http.flavor (#3379) * Fix AspNetCore metrics to use correct value for http.flavor * word better * Logs: Add LogRecordData (#3378) * Add LogRecordData and hook up to LogRecord. * CHANGELOG update. * Code review. * Remove SetHttpFlavor from Http instrumentations (#3381) * Asp.Net Core trace instrumentation to populate http schema tag (#3392) * Try asp.net core tests with inproc server (#3394) * Dedupe IsPackable (#3398) * Remove AspNet and AspNet.TelemetryHttpModule instrumentation projects (#3397) * Handle possible exception when initializing the default service name (#3405) * HttpClient: Invoke Enrich when SocketException = HostNotFound (#3407) * Add & use ConfigureResource API. (#3307) Co-authored-by: tyler jago <[email protected]> Co-authored-by: Robert Pająk <[email protected]> Co-authored-by: Cijo Thomas <[email protected]> Co-authored-by: Timothy Mothra <[email protected]> Co-authored-by: Mikel Blanchard <[email protected]> Co-authored-by: Reiley Yang <[email protected]> Co-authored-by: Utkarsh Umesan Pillai <[email protected]> Co-authored-by: Christian Neumüller <[email protected]>

Addjaeger (#7)

9c19f44

* added jaeger progagator with tests * updated changelog * moved comment to unreleased

notcool11 requested a review from a team May 26, 2022 16:11

reyang approved these changes May 26, 2022

View reviewed changes

alanwest reviewed May 26, 2022

View reviewed changes

src/OpenTelemetry.Extensions.Propagators/CHANGELOG.md Outdated Show resolved Hide resolved

alanwest approved these changes May 26, 2022

View reviewed changes

cijothomas reviewed May 27, 2022

View reviewed changes

src/OpenTelemetry.Extensions.Propagators/JaegerPropagator.cs Outdated Show resolved Hide resolved

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

reyang removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jun 3, 2022

Update CHANGELOG.md

afd7a88

nit from PR comments

another merge from otel main

bc9db4d

pellared reviewed Jun 8, 2022

View reviewed changes

src/OpenTelemetry.Extensions.Propagators/CHANGELOG.md Outdated Show resolved Hide resolved

moved jaegerpropagator comment to unreleased

341fb7e

pellared approved these changes Jun 8, 2022

View reviewed changes

src/OpenTelemetry.Extensions.Propagators/CHANGELOG.md Outdated Show resolved Hide resolved

Update src/OpenTelemetry.Extensions.Propagators/CHANGELOG.md

91415d0

Co-authored-by: Robert Pająk <[email protected]>

cijothomas merged commit 1da5b86 into open-telemetry:main Jun 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Jaeger Propagator to Opentelemetry.Extensions.Propagators #3309

Added Jaeger Propagator to Opentelemetry.Extensions.Propagators #3309

notcool11 commented May 26, 2022 •

edited

Loading

linux-foundation-easycla bot commented May 26, 2022 •

edited

Loading

reyang commented May 26, 2022

notcool11 commented May 26, 2022

cijothomas commented May 26, 2022

reyang left a comment

codecov bot commented May 26, 2022 •

edited

Loading

github-actions bot commented Jun 3, 2022

notcool11 commented Jun 7, 2022

pellared commented Jun 8, 2022

notcool11 commented Jun 8, 2022

notcool11 commented Jun 8, 2022

pellared commented Jun 8, 2022

notcool11 commented Jun 8, 2022

notcool11 commented Jun 8, 2022

pellared commented Jun 8, 2022 •

edited

Loading

notcool11 commented Jun 8, 2022

notcool11 commented Jun 8, 2022

Added Jaeger Propagator to Opentelemetry.Extensions.Propagators #3309

Added Jaeger Propagator to Opentelemetry.Extensions.Propagators #3309

Conversation

notcool11 commented May 26, 2022 • edited Loading

Changes

linux-foundation-easycla bot commented May 26, 2022 • edited Loading

reyang commented May 26, 2022

notcool11 commented May 26, 2022

cijothomas commented May 26, 2022

reyang left a comment

Choose a reason for hiding this comment

codecov bot commented May 26, 2022 • edited Loading

Codecov Report

github-actions bot commented Jun 3, 2022

notcool11 commented Jun 7, 2022

pellared commented Jun 8, 2022

notcool11 commented Jun 8, 2022

notcool11 commented Jun 8, 2022

pellared commented Jun 8, 2022

notcool11 commented Jun 8, 2022

notcool11 commented Jun 8, 2022

pellared commented Jun 8, 2022 • edited Loading

notcool11 commented Jun 8, 2022

notcool11 commented Jun 8, 2022

notcool11 commented May 26, 2022 •

edited

Loading

linux-foundation-easycla bot commented May 26, 2022 •

edited

Loading

codecov bot commented May 26, 2022 •

edited

Loading

pellared commented Jun 8, 2022 •

edited

Loading