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.

Sign up for GitHub

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

Jump to bottom

Extended tracer #6017

Merged
merged 21 commits into from
Dec 7, 2023
Merged

Extended tracer #6017

merged 21 commits into from
Dec 7, 2023

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Nov 27, 2023 •
edited
Loading

Resolves #4610

@zeitlinger zeitlinger requested a review from a team November 27, 2023 12:58
@codecov Codecov
Copy link

codecov bot commented Nov 27, 2023 •
edited
Loading

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (f9e0acb) 91.17% compared to head (f99f7ed) 91.18%.
Report is 22 commits behind head on main.

Files Patch % Lines
...ubator/propagation/ExtendedContextPropagators.java 72.22% 2 Missing and 3 partials ⚠️
...sion/incubator/propagation/CaseInsensitiveMap.java 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #6017      +/-   ##
============================================
+ Coverage     91.17%   91.18%   +0.01%     
- Complexity     5613     5694      +81     
============================================
  Files           616      628      +12     
  Lines         16564    16674     +110     
  Branches       1642     1643       +1     
============================================
+ Hits          15102    15205     +103     
- Misses         1013     1015       +2     
- Partials        449      454       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jack-berg
jack-berg reviewed Nov 28, 2023
View reviewed changes
extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/Propagation.java Outdated Show resolved Hide resolved
extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/Propagation.java Outdated Show resolved Hide resolved
extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/Propagation.java Outdated Show resolved Hide resolved
extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/Propagation.java Outdated Show resolved Hide resolved
extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/Propagation.java Outdated Show resolved Hide resolved
...sions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/ExtendedTracer.java Outdated Show resolved Hide resolved
...sions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/ExtendedTracer.java Show resolved Hide resolved
...sions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/ExtendedTracer.java Outdated Show resolved Hide resolved
extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/README.md Outdated Show resolved Hide resolved
extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/SpanCallback.java Outdated Show resolved Hide resolved
@zeitlinger
Copy link
Member Author

Thanks a lot for the detailed review @jack-berg!

Let me recap the history of this feature:

  1. all static methods
  2. after some discussion, OpenTelemetry and Tracer were injected
  3. when moving this PR here, I added implements Tracer
  4. removing OpenTelemetry as instance again would affect five public methods that are related to propagation - but that's fine for me

Once we've agreed on this approach, I'll implement all requested changes.

@jack-berg
Copy link
Member

This would resolve #4610, correct @zeitlinger?

zeitlinger and others added 8 commits December 1, 2023 17:59
@zeitlinger
- provide propagators as params instead of otel instance
27fb8d3 
- use factory method instead of ctor
- remove getTextMapPropagationContext from ExtendedTracer as it was only delegating
@zeitlinger
make extractTextMapPropagationContext public, because it's in a separ…
1a1a082 
…ate class now
@zeitlinger
move Propagation to io.opentelemetry.extension.incubator.propagation
da6b31f
@zeitlinger
remove unneeded suppressions
a7aa285
@zeitlinger @jack-berg
Update extensions/incubator/src/main/java/io/opentelemetry/extension/…
2d1ea95 
…incubator/trace/SpanCallback.java

Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
@zeitlinger @jack-berg
Apply suggestions from code review
a326af1 
Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
@zeitlinger
fix build
5a7b787
@zeitlinger
- use case insensitive map
86620f0 
- return unmodifiable map
@zeitlinger
Copy link
Member Author

zeitlinger commented Dec 1, 2023 •
edited
Loading

@jack-berg I hope I've incorporated every feedback as intended. I'll update the usage examples once you've confirmed.

jack-berg
jack-berg reviewed Dec 1, 2023
View reviewed changes
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of additional comments. I'm aligned at this point.

extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/README.md Outdated Show resolved Hide resolved
extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/README.md Outdated Show resolved Hide resolved
...sions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/ExtendedTracer.java Outdated Show resolved Hide resolved
extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/SpanCallable.java Outdated Show resolved Hide resolved
extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/SpanRunnable.java Outdated Show resolved Hide resolved
...ns/incubator/src/main/java/io/opentelemetry/extension/incubator/propagation/Propagation.java Outdated Show resolved Hide resolved
@trask trask mentioned this pull request Dec 6, 2023
Closed
zeitlinger and others added 3 commits December 7, 2023 20:55
@zeitlinger @jack-berg
Update extensions/incubator/src/main/java/io/opentelemetry/extension/…
ed829f6 
…incubator/trace/ExtendedSpanBuilder.java

Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
@zeitlinger
review comments
943f2a5
@zeitlinger
scope out callWithBaggage for now
2a3dad5
@jack-berg jack-berg merged commit 2b33f9f into open-telemetry:main Dec 7, 2023
18 checks passed
@zeitlinger zeitlinger deleted the extended-tracer branch December 8, 2023 06:32
@zeitlinger zeitlinger mentioned this pull request Dec 8, 2023
Closed
@jack-berg jack-berg mentioned this pull request Jun 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trask trask trask left review comments

@jack-berg jack-berg jack-berg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tracing utilities
3 participants
@zeitlinger @jack-berg @trask