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

java library instrumentation doc #3568

Merged
merged 7 commits into from
Jan 26, 2024

Conversation

nerudadhich
Copy link
Contributor

@nerudadhich nerudadhich commented Nov 17, 2023

@cijothomas
Copy link
Member

@nerudadhich Do you need review from .NET approvers for this or it was accidentally requested..?

@cartermp cartermp removed request for a team November 20, 2023 23:15
@cartermp
Copy link
Contributor

@cijothomas I took the .NET (and semconv) owners off

Copy link
Member

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

Copy link
Member

@svrnm svrnm left a 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

content/en/docs/instrumentation/java/libraries.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/libraries.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/libraries.md Outdated Show resolved Hide resolved
implementation("io.opentelemetry.instrumentation:opentelemetry-java-http-client:1.31.0-alpha");
}
```

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, Please check.

Copy link
Contributor

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?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chalin @trask I have improved with tabbed panes now, Please suggest if any more text need to add for same ?

Copy link
Contributor

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?

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

@svrnm svrnm left a 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!

Copy link
Contributor

@chalin chalin left a 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.

@nerudadhich
Copy link
Contributor Author

@open-telemetry/java-approvers Could you please review ?
cc - @trask

@trask
Copy link
Member

trask commented Jan 10, 2024

hi @nerudadhich, sorry for the delay.

@jeanbisutti can you take a look at this when you have a chance? thx!

content/en/docs/instrumentation/java/libraries.md Outdated Show resolved Hide resolved
implementation("io.opentelemetry.instrumentation:opentelemetry-java-http-client:1.31.0-alpha");
}
```

Copy link
Contributor

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?

Copy link
Contributor

@chalin chalin left a 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.

@chalin chalin self-requested a review January 11, 2024 12:46

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):

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeanbisutti
Copy link
Member

It looks globally good to me. Perhaps this PR could be merged with the last suggestions from @chalin, and the Java library page could be improved with other PRs? @chalin @svrnm @trask What do you think?

Copy link
Contributor

@chalin chalin left a 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.

@svrnm
Copy link
Member

svrnm commented Jan 17, 2024

@nerudadhich you need to resolve the merge conflict locally. The issue comes from the rename of instrumentation to languages via #3761. Let me know if you need help with that.

@open-telemetry/java-approvers please take another look, I think this came a long way and is ready to be merged:-)

@nerudadhich
Copy link
Contributor Author

nerudadhich commented Jan 17, 2024

@svrnm I have addressed the review comments, I think we are ready to merge. Just need help in Links build that is failing.

@jeanbisutti
Copy link
Member

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.

@trask Is it ok for you to merge?

@nerudadhich
Copy link
Contributor Author

@trask Could please confirm if we can merge ? Also @svrnm How can I see report of broken links in local or somewhere so that I can fix it ?

@svrnm
Copy link
Member

svrnm commented Jan 22, 2024

Also @svrnm How can I see report of broken links in local or somewhere so that I can fix it ?

You can run npm run test-and-fix locally to get most problems resolved, or it will dump you the error. You can also do check:links for pure link checking

@svrnm
Copy link
Member

svrnm commented Jan 25, 2024

@open-telemetry/java-approvers please take a look

@svrnm svrnm removed the sig-approval-missing Co-owning SIG didn't provide an approval label Jan 26, 2024
@svrnm svrnm merged commit d51c97f into open-telemetry:main Jan 26, 2024
14 checks passed
@svrnm
Copy link
Member

svrnm commented Jan 26, 2024

Thank you for working through this, @nerudadhich, great contribution!

@nerudadhich
Copy link
Contributor Author

Thank you for working through this, @nerudadhich, great contribution!

Thanks for helping everyone ❤️

jaydeluca pushed a commit to jaydeluca/opentelemetry.io that referenced this pull request Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Libraries doc or section for Java, showing how to set up library instrumentation without an agent
9 participants