-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
java library instrumentation doc #3568
java library instrumentation doc #3568
Conversation
b0af7ed
to
364ed7e
Compare
5bf335e
to
771af68
Compare
2bbf5f2
to
673ec35
Compare
@nerudadhich Do you need review from .NET approvers for this or it was accidentally requested..? |
@cijothomas I took the .NET (and semconv) owners off |
content-modules/semantic-conventions
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to rebase this PR before merging to remove this. @nerudadhich No action needed, this is just a note to maintainers, we tackle this last minute, for future PRs: make sure that you always work from a synced based that has no changes to content-modules ( you can verify with git status
or git diff
before creating commits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Would like to get some feedback from @open-telemetry/java-approvers as well. Here are a few initial comments by me
implementation("io.opentelemetry.instrumentation:opentelemetry-java-http-client:1.31.0-alpha"); | ||
} | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a not here that maven
can be used of course as well and that the relevant dependency here is io.opentelemetry.instrumentation:opentelemetry-java-http-client:1.31.0-alpha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, Please check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@svrnm - shouldn't we use tabbed panes in this case instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would be nice here too: https://opentelemetry.io/docs/instrumentation/java/getting-started/#dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tabbed pane seems file. I'll let @svrnm give the final work as to whether this comment is resolved to his liking.
that would be nice here too: https://opentelemetry.io/docs/instrumentation/java/getting-started/#dependencies
@trask - maybe we can open a separate issue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nerudadhich would it be a lot of effort to make this work with gradle and maven out of the box? If yes, let's keep it as is and have a separate issue for that, if no, feel free to update the PR accordingly. This would than nicely fit with the other existing documentation where we provide both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @svrnm I tired spending some time on this but I am not Maven expert if we can get someone's help would be great or I can still spend more time if we are not in rush.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall lgtm, @open-telemetry/java-approvers PTAL!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in #3568 (comment), this PR needs to be rebased. Since @svrnm has approved, I'm issuing this "request changes" as a safeguard to ensure that the PR doesn't get merged by mistake containing a retrograde of the semconv submodule.
@open-telemetry/java-approvers Could you please review ? |
hi @nerudadhich, sorry for the delay. @jeanbisutti can you take a look at this when you have a chance? thx! |
implementation("io.opentelemetry.instrumentation:opentelemetry-java-http-client:1.31.0-alpha"); | ||
} | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tabbed pane seems file. I'll let @svrnm give the final work as to whether this comment is resolved to his liking.
that would be nice here too: https://opentelemetry.io/docs/instrumentation/java/getting-started/#dependencies
@trask - maybe we can open a separate issue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're getting there! See inline for a tweak to the alert
.
|
||
The following example shows how you can instrument external API calls using | ||
[Java HTTP client library](https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/java-http-client/library): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps mentioned that with Spring Boot the SDK (OpenTelemetry object) can be initialized with the OpenTelemetry starter: https://opentelemetry.io/docs/languages/java/automatic/spring-boot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure @jeanbisutti
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the tweak to the alert is merged, since others are ok (generally speaking) with this update, I'm giving a 👍🏻 to a merge.
I agree with @jeanbisutti that I'd prefer that we address other changes separately.
@nerudadhich you need to resolve the merge conflict locally. The issue comes from the rename of @open-telemetry/java-approvers please take another look, I think this came a long way and is ready to be merged:-) |
65ebe4a
to
d35c625
Compare
@svrnm I have addressed the review comments, I think we are ready to merge. Just need help in |
@trask Is it ok for you to merge? |
Also @svrnm How can I see report of broken links in local or somewhere so that I can fix it ? You can run |
@open-telemetry/java-approvers please take a look |
Thank you for working through this, @nerudadhich, great contribution! |
Thanks for helping everyone ❤️ |
Fixes #2471
Preview: https://deploy-preview-3568--opentelemetry.netlify.app/docs/instrumentation/java/libraries/