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

Added @WithSpan documentation for Java #1178

Merged
merged 4 commits into from
Mar 7, 2022

Conversation

Gaurang-Patel
Copy link
Contributor

@Gaurang-Patel Gaurang-Patel commented Feb 27, 2022

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

I think this is fine to incorporate for now, maybe some tweaks.

The main concern I have is that there's a bit of duplication. This article covers how to annotate the current span, which is fundamentally a manual instrumentation scenario.

It also covers the @WithSpan annotation, which is dope because it's a lightweight way to annotate any method anywhere. But that's also a manual instrumentation scenario.

That said, I understand that it exists as a means to do lightweight, supplementary instrumentation in concert with automatic instrumentation. That context is important and I wouldn't want to fold this into the existing manual instrumentation article without explaining that somehow.

That scenario may also be worth documenting in its own right as well. But I think it's unique to Java, at least for now.

content/en/docs/instrumentation/java/automatic/withspan.md Outdated Show resolved Hide resolved
@cartermp
Copy link
Contributor

cartermp commented Mar 1, 2022

@austinlparker and @chalin what are your thoughts and feels?

@Gaurang-Patel Gaurang-Patel changed the title main: Added @WithSpan documentation for Java Added @WithSpan documentation for Java Mar 3, 2022
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.

Partly based in part on @cartermp's earlier remarks, I'd be inclined to leave automatic.md where it is, and add your new page as, say, annotations.md.

Thoughts @cartermp @austinlparker et all?

Also, see inline comments.

content/en/docs/instrumentation/java/automatic/withspan.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/automatic/withspan.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/automatic/withspan.md Outdated Show resolved Hide resolved

public class MyClass {
@WithSpan
public void MyLogic(@SpanAttribute("parameter1") String parameter1, @SpanAttribute("parameter2") long parameter2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a line break and indenting the arguments, that'll make the use of @SpanAttribute easier to see.

content/en/docs/instrumentation/java/automatic/withspan.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/automatic/withspan.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/automatic/withspan.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/automatic/_index.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/automatic/withspan.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/java/automatic/withspan.md Outdated Show resolved Hide resolved
@chalin
Copy link
Contributor

chalin commented Mar 3, 2022

Thanks for the contribution @Gaurang-Patel! Please see my review comments.

@chalin
Copy link
Contributor

chalin commented Mar 3, 2022

@trask
Copy link
Member

trask commented Mar 3, 2022

So, just to be clear, this is meant to replace https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/manual-instrumentation.md, right? /cc @trask

sound good to me 👍 cc: @open-telemetry/java-instrumentation-approvers

@Gaurang-Patel
Copy link
Contributor Author

Hi @chalin, @cartermp, I've updated pr as per the review comment, Please review and do let me know if you have any other suggestions.

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

I like it!

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.

Well done. LGTM

@chalin
Copy link
Contributor

chalin commented Mar 7, 2022

So, just to be clean, this is meant to replace https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/manual-instrumentation.md, right? /cc @trask

sound good to me 👍 cc: @open-telemetry/java-instrumentation-approvers

@trask et all: see open-telemetry/opentelemetry-java-instrumentation#5516

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.

Java - Manual Instrumentation - add @WithSpan annotation
5 participants