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 withParent option to annotation @WithSpan #13112

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

xiepuhuan
Copy link
Contributor

Related to #1036

@xiepuhuan xiepuhuan requested a review from a team as a code owner January 27, 2025 07:56
@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Jan 27, 2025
@xiepuhuan
Copy link
Contributor Author

xiepuhuan commented Jan 27, 2025

The smoke test of grpc failed. Because I modified the @WithSpan annotation of instrumentation-annotations, and the grpc smoke test image uses @WithSpan, I need to rely on the new smoke test grpc image version.
What should I do? @trask help me

@laurit
Copy link
Contributor

laurit commented Jan 27, 2025

The smoke test of grpc failed. Because I modified the @WithSpan annotation of instrumentation-annotations, and the grpc smoke test image uses @WithSpan, I need to rely on the new smoke test grpc image version. What should I do? @trask help me

You got it wrong. The test is failing because after your changes the instrumentation does not work any more for old version of the @WithSpan annotation that does not have the withParent attribute. This is a bug in your PR.

@xiepuhuan
Copy link
Contributor Author

xiepuhuan commented Jan 28, 2025

The smoke test of grpc failed. Because I modified the @WithSpan annotation of instrumentation-annotations, and the grpc smoke test image uses @WithSpan, I need to rely on the new smoke test grpc image version. What should I do? @trask help me

You got it wrong. The test is failing because after your changes the instrumentation does not work any more for old version of the @WithSpan annotation that does not have the withParent attribute. This is a bug in your PR.

@laurit Thank you for your reply. I want to use reflection to determine whether the withParent method exists to fix this bug and achieve backward compatibility. Do you have any suggestions for this?

* <p>If set to {@code false}, the created span will use {@link Context#root()} as its parent,
* starting a new, independent trace.
*/
boolean withParent() default true;
Copy link
Member

Choose a reason for hiding this comment

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

I kind of like the name proposed in #5837: noParent()

at first I thought I liked something like newTrace(), but concerned people will overuse it thinking that if they don't then it won't create a span if there's no existing trace

cc @open-telemetry/java-instrumentation-approvers any thoughts on naming?

Copy link
Contributor Author

@xiepuhuan xiepuhuan Jan 30, 2025

Choose a reason for hiding this comment

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

root() or rootSpan()?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, what I was trying to avoid is that people may think using root() or rootSpan() is the only way to create a root span, when creating a root span is already the default if there's not an existing parent span already

some other options...

  • withoutParent()
  • breakFromParent()
  • ignoreParent()

Copy link
Member

Choose a reason for hiding this comment

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

another option

  • inheritContext() default true

which points to it being more than just lack of parent span

Copy link
Contributor Author

Choose a reason for hiding this comment

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

another option

  • inheritContext() default true

which points to it being more than just lack of parent span

I think this is good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test native This label can be applied to PRs to trigger them to run native tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants