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.

Sign up for GitHub

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

Jump to bottom

x-datadog-tags propagation and upstream_services tracking #3294

Merged
merged 22 commits into from Jan 24, 2022
Merged

x-datadog-tags propagation and upstream_services tracking #3294

merged 22 commits into from Jan 24, 2022

Conversation

ygree
Copy link
Contributor

@ygree ygree commented Dec 27, 2021 •
edited

What Does This Do

  • Base64 support for Java 7 (needed for service name encoding)
  • On sampling priority changes update (or add if this is the first service) the _dd.p.upstream_services tag to include the current decision
  • Add an option to enable upstream_services tracking and make it off by default
    • RFC states opposite - enabled by default (see Requirements 2)
  • DDSpanContext - add a new field to hold the upstream_services value. This value will also be added to the tags
  • Get the value of the _dd.p.upstream_services tag from the incoming headers. The tag may not exist or it may have an empty value. Empty values do not need to be propagated. The value will be encoded as described in the RFC document.
    Currently, if there is no change to upstream_services then x-datadog-tags will stay untouched and propagated as-is
  • Decode the value and add it to the local root span tags.
  • On client calls encode the _dd.p.upstream_services tag and add it to the outgoing headers
  • When x-datadog-tags value reached its limit (512 (configurable)), tracer should do the following things
  • Drops all tags, and doesn’t send x-datadog-tags header
  • Logs a recommendation for increasing the limit with error level
  • Sets _dd.propagation_error:max_size in trace tags to allow backend to know
  • Transporting to Backend
  • For older endpoints (v01, v02, v03, v04, v05), we set trace tags in Meta of the first span of each chunk.
  • For newer endpoints (v06, v07), we set trace tags in Tags of TraceChunk structure.
    Does dd-trace-java even support v06, v07?
  • Validate and warn about malformed x-datadog-tags

Motivation

[RFC] Trace Tags Propagation in Distributed Traces.

Additional Notes

Implementation plan

bantonsson
bantonsson reviewed Dec 28, 2021
View reviewed changes
.../src/main/java/datadog/trace/instrumentation/kafka_clients/KafkaConsumerInstrumentation.java Outdated
@@ -40,7 +40,7 @@ public KafkaConsumerInstrumentation() {
packageName + ".TracingIterator",
packageName + ".TracingList",
packageName + ".TracingListIterator",
packageName + ".Base64Decoder"
"datadog.trace.util.Base64Decoder" // TODO: can be removed when JDK7 is dropped
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to add this now that the class is part of the internal API.

bantonsson
bantonsson reviewed Dec 28, 2021
View reviewed changes
internal-api/src/test/groovy/datadog/trace/util/Base64Test.groovy Outdated
originalString | padding
"" | true
"a" | true
"ab" | true
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good to have some expected output here as well. I once came across a base58check encoder/decoder that was very good at talking to itself, but wasn't compliant when tested against other encoders/decoders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

@ygree ygree force-pushed the upstream-services-tag branch 5 times, most recently from 9bfa394 to 9a23c3b Compare December 30, 2021 17:52
@ygree ygree force-pushed the upstream-services-tag branch 5 times, most recently from 21fe7d5 to 6e81a99 Compare January 6, 2022 01:09
@ygree
Base64 encoding for Java 7
d431a0e 
Move Base64Decoder to internal-api
Introduce Base64Encoder and a test

Signed-off-by: Yury Gribkov <yury.gribkov@gmail.com>
@ygree
Extract and inject datadog-x-tags with a dummy DatadogTags implementa…
e1bb20e 
…tion.

Signed-off-by: Yury Gribkov <yury.gribkov@gmail.com>
@ygree
Implement DatadogTags updateUpstreamServices and encode logic
08ea807 
Signed-off-by: Yury Gribkov <yury.gribkov@gmail.com>
@ygree
Keep only one sampling decision per SpanContext
6534c9b 
Signed-off-by: Yury Gribkov <yury.gribkov@gmail.com>
@ygree
Update empty upstream_services tag without adding extra separator
f701bee 
Signed-off-by: Yury Gribkov <yury.gribkov@gmail.com>
@ygree
Add missing tags injection/extraction tests
d14bbe8 
Signed-off-by: Yury Gribkov <yury.gribkov@gmail.com>
@ygree
Fix instrumentation test assertions for upstream_services
53631b9
@ygree
Clean up DatadogTags
2959c8c
@ygree
Introduce trace.upstream.services.tracking.enabled
5f556ae
@ygree
Introduce trace.datadog.tags.limit
bab3479
@ygree
Copy link
Contributor Author

ygree commented Jan 6, 2022

Instead of just adding a control point for the upstream_services part of x-datadog-tags should we add an option to turn off the x-datadogs-tags parsing, etc. completely?

@charliegracie I think you're right, according to the RFC: "Customers can disable the propagation of these additional attributes, but it’s enabled by default". I'll change it accordingly.

@ygree
Copy link
Contributor Author

ygree commented Jan 6, 2022

Instead of just adding a control point for the upstream_services part of x-datadog-tags should we add an option to turn off the x-datadogs-tags parsing, etc. completely?

@charliegracie I think you're right, according to the RFC: "Customers can disable the propagation of these additional attributes, but it’s enabled by default". I'll change it accordingly.

Looks like this feature is not required according to the discussion in the RFC document. I'll remove this feature for now then.

devinsba
devinsba reviewed Jan 10, 2022
View reviewed changes
Copy link
Contributor

@devinsba devinsba left a comment •
edited

Choose a reason for hiding this comment

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

I was under the impression that when "updating" you append the new decision to the old one. Maybe I misunderstood the RFC but I don't see that happening here

dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java Outdated
Comment on lines 217 to 218
String newServiceName = trace.getTracer().mapServiceName(serviceName);
this.serviceName = newServiceName;
Copy link
Contributor

@devinsba devinsba Jan 10, 2022 •
edited

Choose a reason for hiding this comment

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

Looks like this change can be reverted

dd-trace-core/src/test/groovy/datadog/trace/core/propagation/DatadogHttpExtractorTest.groovy
import static datadog.trace.core.propagation.DatadogHttpCodec.SAMPLING_PRIORITY_KEY
import static datadog.trace.core.propagation.DatadogHttpCodec.SPAN_ID_KEY
import static datadog.trace.core.propagation.DatadogHttpCodec.TRACE_ID_KEY
import static datadog.trace.core.propagation.DatadogHttpCodec.*
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like you are missing the intellij config to not make these "*" imports. I set mine to 100 in my config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These imports seemed bulky to me as it imports more than 200 static members, so I wanted to improve it. If it's something that against our code style I'm okay with reverting this change.

@ygree
Copy link
Contributor Author

ygree commented Jan 10, 2022 •
edited

I was under the impression that when "updating" you append the new decision to the old one. Maybe I misunderstood the RFC but I don't see that happening here

DatadogTags implements logic to append a new decision to the propagated ones. When it injects or sends it to the agent it will include any new sampling decision to already propagated upstread_services and possibly other x-datadog-tags. If setSamplingPriority is called for the same service more than once then the last one will kept. Can you please point me to RFC where it states opposite. I'd like to fix it if the current implementation is not aligned with the RFC.

@ygree ygree requested review from bantonsson, devinsba and charliegracie and removed request for charliegracie January 11, 2022 19:01
@ygree ygree self-assigned this Jan 13, 2022
@ygree ygree changed the title upstream_services tag propagation x-datadog-tags propagation and upstream_services tracking Jan 20, 2022
@ygree ygree merged commit e30573e into DataDog:master Jan 24, 2022
@bantonsson bantonsson added this to the 0.94.0 milestone Feb 24, 2022
mcculls added a commit that referenced this pull request Mar 10, 2022
@mcculls
Revert "Merge pull request #3294 from ygree/upstream-services-tag"
058bf6d 
This reverts commit e30573e, reversing
changes made to d7fde2a.
mcculls added a commit that referenced this pull request Mar 10, 2022
@mcculls
Revert "Merge pull request #3294 from ygree/upstream-services-tag"
8c16de2 
This reverts commit e30573e, reversing
changes made to d7fde2a.
mcculls added a commit that referenced this pull request Mar 10, 2022
@mcculls
Revert "Merge pull request #3294 from ygree/upstream-services-tag"
76b7e0a 
This reverts commit e30573e, reversing
changes made to d7fde2a.
mcculls added a commit that referenced this pull request Mar 10, 2022
@mcculls
Revert "Merge pull request #3294 from ygree/upstream-services-tag" (#…
76e41d3 
…3432)

This reverts commit e30573e, reversing
changes made to d7fde2a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devinsba devinsba devinsba approved these changes

@charliegracie charliegracie charliegracie approved these changes

@bantonsson bantonsson Awaiting requested review from bantonsson

Assignees

@ygree ygree

Labels
None yet
Projects
None yet
Milestone
0.94.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ygree @devinsba @bantonsson @charliegracie