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

add extended tracer module #759

Merged
merged 33 commits into from
Nov 8, 2023
Merged

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Feb 23, 2023

The Opentelemetry APIs are rather difficult to use - therefore I've developed the following utility method, which is part of a company internal library - so it has already been tested by some users.

Based on this PR, I'm now creating a PR here.

@zeitlinger
Copy link
Member Author

@trask I hope this PR makes more sense now - I've kept the idea of being able to wrap a Tracer - but I've also kept the static methods in the Tracing class.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

I'm a little hesitant to introduce too much syntactic sugar on top of opentelemetry-api, since that creates more ways to do the same thing, which could cause confusion

at the same time, it makes sense to have somewhere to experiment with things that we might want to add into the opentelemetry-api itself

we also have opentelemetry-instrumentation-api, which possibly has a little bit of overlap

I left a couple of initial comments, overall I think it might help to pair the PR down and just focus on the Tracing class at least initially

@zeitlinger zeitlinger force-pushed the extended-tracer branch 2 times, most recently from 1326088 to 150b2be Compare July 27, 2023 14:18
@zeitlinger
Copy link
Member Author

we also have opentelemetry-instrumentation-api, which possibly has a little bit of overlap

I checked Instrumenter but didn't see anything that could be reused.

I left a couple of initial comments, overall I think it might help to pair the PR down and just focus on the Tracing class at least initially

I like that idea and have adjusted the PR accordingly 😄

@zeitlinger
Copy link
Member Author

@trask @jkwatson @jack-berg as discussed yesterday, I've removed GlobalTracer, injected Tracer into Tracing, and added before/after examples.

zeitlinger and others added 24 commits November 7, 2023 11:10
- inject Tracer into Tracing, making it easier to use
…Tracing.java

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
…ThrowingSupplier.java

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
…Tracing.java

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
- extract Propagation class with static methods
- extract Propagation class with static methods
- extract Propagation class with static methods
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
@zeitlinger
Copy link
Member Author

@trask build is passing now 😄

@trask trask merged commit 2983ea7 into open-telemetry:main Nov 8, 2023
14 checks passed
@trask
Copy link
Member

trask commented Nov 8, 2023

thanks @zeitlinger for your patience and incorporating all the feedback!

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

Successfully merging this pull request may close these issues.

None yet