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

Fix Java manual instrumentation with Spring Boot app #3656

Conversation

PerfectSlayer
Copy link
Contributor

Hi OpenTelemetry team 👋

Following a dependency management issue using Java Spring Boot app and Gradle build system, I updated the Java Manual Instrumentation documentation page.

This PR proposes a fix to #3613 and the rationale for its content (using OTel BOM import rather than removing Spring dependency management pluging) is explained in my comment: open-telemetry/opentelemetry-java#6018 (comment).

It's my first contribution to this repository so feel free to provide guidance if I miss something.
I have built the site locally and tested it using npm run test. It seemed fine 😇

Note

In addition, I updated the highlight sections, fix semconv versions and restore a missing semi-column.

@PerfectSlayer PerfectSlayer requested review from a team December 7, 2023 13:09
@PerfectSlayer PerfectSlayer changed the title Fix munual instrumentation with Spring Boot app Fix Java manual instrumentation with Spring Boot app Dec 7, 2023
@PerfectSlayer PerfectSlayer force-pushed the bbujon/fix-java-manual-instrumentation branch from 8c81894 to ee1828c Compare December 7, 2023 13:09
@svrnm svrnm added the sig:java label Dec 7, 2023
@svrnm
Copy link
Member

svrnm commented Dec 7, 2023

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

@jeanbisutti
Copy link
Member

@PerfectSlayer There are two ways to manage a BOM with Gradle and Spring Boot. Could you please also document the way with the Gradle’s native bom support (as I did on this PR for the Spring Boot page)?

@jeanbisutti
Copy link
Member

@PerfectSlayer Could you also please add documentation on how to configure the things with Maven (as I did on #3672 for the Spring Boot page)?

@svrnm
Copy link
Member

svrnm commented Dec 12, 2023

@open-telemetry/java-approvers PTAL

@PerfectSlayer
Copy link
Contributor Author

@jeanbisutti Sure, I was waiting for #3672 to be reviewed first.

@PerfectSlayer PerfectSlayer force-pushed the bbujon/fix-java-manual-instrumentation branch 2 times, most recently from 810ba6d to 4be920f Compare December 20, 2023 10:30
@PerfectSlayer PerfectSlayer requested a review from svrnm December 20, 2023 10:31
@PerfectSlayer
Copy link
Contributor Author

It pushed some changes to reflect BOM documentation from @jeanbisutti
I also moved a paragraph (throughout this documentation you will add dependencies.…) from the Gradle tab below, to be accessible for Maven users too, and removed duplicated maven dependencies to stay consistent (only adding SDK on installing SDK chapter, like autoconfigure).

@cartermp
Copy link
Contributor

@PerfectSlayer if you run npm run fix:format then this should be good.

@PerfectSlayer PerfectSlayer force-pushed the bbujon/fix-java-manual-instrumentation branch from 7090629 to 21ba361 Compare December 20, 2023 15:40
@PerfectSlayer
Copy link
Contributor Author

PerfectSlayer commented Dec 20, 2023

@cartermp I run it and amend the commit it fixed.

Oh and sorry for both typos... Pretty exhausted by the end of the year 😫

@cartermp
Copy link
Contributor

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

@cartermp cartermp added the sig-approval-missing Co-owning SIG didn't provide an approval label Dec 20, 2023
@PerfectSlayer
Copy link
Contributor Author

Hey there! Happy New Year 🎉

Coming to check up on things. Is there anything I can do on my side to get this PR progress?

@trask
Copy link
Member

trask commented Jan 4, 2024

we are good to go from the Java SIG side with @laurit and @jeanbisutti's approvals

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.

Left a handful of comments that aren't necessarily related to the changes @PerfectSlayer makes, but didn't seem right to let them continue to go unnoticed.

}
```

Throughout this documentation you will add dependencies. For a full list of
artifact coordinates, see [releases]. For semantic convention releases, see
Copy link
Member

Choose a reason for hiding this comment

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

Throughout this documentation you will add dependencies. For a full list of
artifact coordinates, see [releases].

I like this section near the top - maybe even the first line in the dependency management section. Starting with the bom feels strange without first establishing which artifacts the bom aligns.

If you instrument a Java app, install the dependencies for the OpenTelemetry
SDK.
If you instrument a Java app, install the dependencies for the OpenTelemetry SDK
in addition to the dependencies for the OpenTelemetry API.
Copy link
Member

Choose a reason for hiding this comment

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

The phrasing here is odd. Your change is fine but I wonder if we should correct it.

If you instrument a Java app, install the dependencies for the OpenTelemetry SDK

That's a really poor introduction to the SDK. Maybe something like:

"The OpenTelemetry API provides a set of interfaces for collecting telemetry, but the data is dropped without an implementation. The OpenTelemetry SDK is the implementation of the OpenTelemetry API provided by OpenTelemetry. To use it, begin by installing dependencies:"

content/en/docs/instrumentation/java/manual.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/manual.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/manual.md Outdated Show resolved Hide resolved
implementation("io.opentelemetry:opentelemetry-sdk-metrics:{{% param vers.otel %}}");
implementation("io.opentelemetry:opentelemetry-exporter-logging:{{% param vers.otel %}}");
implementation("io.opentelemetry:opentelemetry-semconv:{{% param vers.otel %}}-alpha");
implementation("io.opentelemetry:opentelemetry-api");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
implementation("io.opentelemetry:opentelemetry-api");

I believe your intent is to omit opentelemetry-api here since its included above.

content/en/docs/instrumentation/java/manual.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/manual.md Outdated Show resolved Hide resolved
@svrnm
Copy link
Member

svrnm commented Jan 18, 2024

@PerfectSlayer can you take a look at the review comments?

@PerfectSlayer
Copy link
Contributor Author

Sure will do this tomorrow or next week 👍

@zeitlinger
Copy link
Member

In addition, I updated the highlight sections

what is the intention of the highlights?

@svrnm svrnm removed the sig-approval-missing Co-owning SIG didn't provide an approval label Jan 25, 2024
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 comment.

This will need to be rebased.

content/en/docs/instrumentation/java/manual.md Outdated Show resolved Hide resolved
@PerfectSlayer PerfectSlayer force-pushed the bbujon/fix-java-manual-instrumentation branch from 509d9c3 to c86bb8a Compare February 2, 2024 12:30
@PerfectSlayer PerfectSlayer force-pushed the bbujon/fix-java-manual-instrumentation branch from c86bb8a to cbdd65b Compare February 2, 2024 12:57
@PerfectSlayer PerfectSlayer force-pushed the bbujon/fix-java-manual-instrumentation branch from cbdd65b to a45d5c0 Compare February 2, 2024 13:01
@PerfectSlayer
Copy link
Contributor Author

I rebased the branch and should have taken all the review comments into account.

@jeanbisutti
Copy link
Member

LGTM

Thanks a lot @PerfectSlayer!

It would remain the comment from Gregor: #3656 (comment) @zeitlinger, could you please elaborate a bit your question?

@PerfectSlayer PerfectSlayer requested a review from svrnm February 5, 2024 07:40
@svrnm
Copy link
Member

svrnm commented Feb 5, 2024

@PerfectSlayer can you resolve the conflict?

@PerfectSlayer
Copy link
Contributor Author

@svrnm I just did using GH Web UI.

@cartermp cartermp merged commit 8058738 into open-telemetry:main Feb 5, 2024
14 checks passed
@PerfectSlayer PerfectSlayer deleted the bbujon/fix-java-manual-instrumentation branch February 5, 2024 13:23
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.

9 participants