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 documentation on the OTel Spring Boot starter #3377

Merged
merged 29 commits into from
Nov 14, 2023
Merged

Add documentation on the OTel Spring Boot starter #3377

merged 29 commits into from
Nov 14, 2023

Conversation

jeanbisutti
Copy link
Member

@jeanbisutti jeanbisutti commented Oct 13, 2023

@jeanbisutti jeanbisutti requested review from a team October 13, 2023 14:39
@jeanbisutti
Copy link
Member Author

This PR could be merged after an OTel Java instrumentation release contains open-telemetry/opentelemetry-java-instrumentation#9625

@trask
Copy link
Member

trask commented Oct 13, 2023

This PR could be merged after an OTel Java instrumentation release contains open-telemetry/opentelemetry-java-instrumentation#9625

since it will be ~ a month before next OTel Java Instrumentation release, what do you think of submitting a PR without the DataSource docs first and we can merge that, and a follow-up PR adding in the DataSource docs?

@jeanbisutti
Copy link
Member Author

This PR could be merged after an OTel Java instrumentation release contains open-telemetry/opentelemetry-java-instrumentation#9625

since it will be ~ a month before next OTel Java Instrumentation release, what do you think of submitting a PR without the DataSource docs first and we can merge that, and a follow-up PR adding in the DataSource docs?

Documentation on JDBC instrumentation removed

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.

Thanks for taking the time to write this down @jeanbisutti! I provided some change requests below.

My major point is that people not familiar with OpenTelemetry might get confused what the differences between this approach and the agent are. This document should either link to another page clarifying that or doing it directly.

content/en/docs/instrumentation/java/springboot.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/springboot.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/springboot.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/springboot.md Outdated Show resolved Hide resolved
@svrnm svrnm added the sig:java label Oct 23, 2023
@jeanbisutti
Copy link
Member Author

Hi @svrnm!

Many thanks for your review!

I have worked on addressing your feedback. Could you please review the last changes?

Could you please provide some guidance to fix the CI issues?

Thank you!

@jeanbisutti jeanbisutti requested a review from svrnm October 30, 2023 12:04
@svrnm
Copy link
Member

svrnm commented Nov 6, 2023

I have worked on addressing your feedback. Could you please review the last changes?

I was out of office last week, so apologies for the delay, let me take another pass

Could you please provide some guidance to fix the CI issues?

do not worry too much about them, some of them can be fixed when the PR is ready to be merged.

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.

a few more comments.

content/en/docs/instrumentation/java/springboot.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/springboot.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/springboot.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/springboot.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/springboot.md Outdated Show resolved Hide resolved
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.

See inline suggestions that replace javaInstrumentationVersion with vers.instrumentation.

@jeanbisutti jeanbisutti requested a review from svrnm November 14, 2023 13:45
@jeanbisutti
Copy link
Member Author

@chalin @svrnm Is there anything else to do on my side?

@chalin
Copy link
Contributor

chalin commented Nov 14, 2023

/fix:format

@chalin
Copy link
Contributor

chalin commented Nov 14, 2023

@chalin @svrnm Is there anything else to do on my side?

Looking now.

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.

A (hopefully final) copyedit pass, adding tabbed panes too.

@jeanbisutti
Copy link
Member Author

Thank you @svrnm and @chalin for the review and the help.

@svrnm svrnm merged commit 2b18b55 into open-telemetry:main Nov 14, 2023
14 checks passed
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.

4 participants