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

Only root span gets AppD attributes set #2247

Merged
merged 4 commits into from
Mar 25, 2025

Conversation

robsunday
Copy link
Contributor

As mentioned in signalfx/gdi-specification#338, 4 AppD context headers should be transferred into corresponding span attributes only for the root span.

Related to #2198

@robsunday robsunday requested review from a team as code owners March 24, 2025 09:53
if (ctx.getTierId() != null) {
span.setAttribute(APPD_ATTR_TIER, ctx.getTierId());
// Set attributes only for the root span
if (span.getParentSpanContext() == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect this is not what you really want. Instead of the root span you want the local root span, the first span in the trace for this service. Secondly can span.getParentSpanContext() be null at all? I'd guess that it would return an invalid span context (trace and span id are all zeros) instead. For ideas see https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/8b7e4f2bedc08cdcda2629b2b486bc9b8fd40851/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/LocalRootSpan.java#L49

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion. Thanks!

@breedx-splk breedx-splk merged commit 64a7eb3 into signalfx:main Mar 25, 2025
26 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants