Refactor:Move distributed propagation to Contrib #2352
Conversation
github-actions
bot
added
appsec
marcotc
force-pushed
the
refactor-propagation-2
branch
from
dc9b25f
to
72bc3a6
Compare
marcotc
force-pushed
the
refactor-propagation-2
branch
from
dc18811
to
faadfb9
Compare
marcotc
force-pushed
the
refactor-propagation-2
branch
from
faadfb9
to
f8ecf3a
Compare
| require_relative '../../propagation/grpc' | ||
| require_relative 'datadog_interceptor' |
There was a problem hiding this comment.
These were just unnecessary and actually the responsibility of intercept_with_datadog.rb.
| .with(Datadog::Tracing::Distributed::Headers::Ext::HTTP_HEADER_TRACE_ID, trace_id.to_s) | ||
| .with('x-datadog-trace-id', trace_id.to_s) |
There was a problem hiding this comment.
Instead of fixing the namespace changes, I chose to inline the string values here: it makes tests more robust and easier to read.
|
|
| # HTTP headers one should set for distributed tracing. | ||
| # These are cross-language (eg: Python, Go and other implementations should honor these) | ||
| # DEV-2.0: This module only exists for backwards compatibility with the public API. It should be removed. | ||
| # @deprecated use [Datadog::Tracing::Distributed::Ext] | ||
| # @public_api |
There was a problem hiding this comment.
Minor: Would it make sense to remove the @public_api now that we decided to deprecate this module? We wouldn't break any code, and would further signal that you shouldn't be looking in here.
There was a problem hiding this comment.
We can't, as it will completely hide it from our public API docs and that's technically a breaking change.
Best I can do is a stern comment for now :)
There was a problem hiding this comment.
... and that's technically a breaking change.
Is it though? I'd claim a breaking change is one where you upgrade from version N to version N+1 and your existing code stops working.
In this case, your code wouldn't stop working. And it's not like the lib isn't opensource, and we don't ship the source to customers, so they can always still reference it; we just don't present it in the docs.
(I'm fine with not changing it, just saying that I don't think it's that much of a problem if we did! :P)
There was a problem hiding this comment.
I'm pretty sure y'all implemented a breaking change anyway.
I've got a bot PR that broke trying to Bump [ddtrace](https://github.com/DataDog/dd-trace-rb) from 1.6.1 to 1.7.0. because NameError: uninitialized constant Datadog::Tracing::Distributed::Headers
Also, I could be wrong, but I don't think Datadog::Tracing::Distributed::Ext exists.
If it does, I'd appreciate a reference to it. ^^
There was a problem hiding this comment.
Apologies for the delay in answering @Judahmeek.
Most modules under Datadog::Tracing::Distributed::Headers are currently not considered part of the public API, as they are not tagged with @public_api.
Nevertheless, we have broken your setup, which is definitely not what we intended!
Let us know if we can help you move to 1.7.0+, and also, if you can, can you share how you're using these modules so we can hopefully take that into consideration for future changes? That will help us evaluate the current API and if there's something we're currently not exposing and that we should.
| unless trace_digest.trace_id == extracted_trace_digest.trace_id \ | ||
| && trace_digest.span_id == extracted_trace_digest.span_id |
There was a problem hiding this comment.
Should we also look to see if non-datadog digests match the datadog digest? This was not in the code prior to the refactoring either, but I'm curious if it would make sense.
There was a problem hiding this comment.
I'm making larger changes to this logic, including how we prioritize the Datadog digest over others in a follow up PR. I'd rather touch this part there. I believe this logic will change quite a bit.
Co-authored-by: Ivo Anjo <ivo.anjo@datadoghq.com>
Co-authored-by: Ivo Anjo <ivo.anjo@datadoghq.com>
| @@ -73,12 +73,10 @@ | |||
| context 'when value is' do | |||
| [ | |||
| [nil, nil], | |||
| ['not a number', nil], | |||
| ['1.to_i returns 1', nil], | |||
There was a problem hiding this comment.
Minor: I'm not entirely sure it's clear what we're testing here (and to be fair, I think this was part of the problem of the old test -- it was actually not clear what it was testing).
I suggest perhaps ['1 2 3', nil], # '1 2 3'.to_i => 1 and we have extra logic to avoid this.
(Also applies below)
The
Datadog::Tracing::Distributednamespace lives in the coreDatadog::Tracingnamespace, but a lot of its logic is propagation style-specific. We have two propagation styles, based on different protocols: HTTP Headers and gRPC metadata.The current implementation contains a lot of complex, duplicated logic between these two styles. Also, some features from the HTTP headers implementation are missing from the gRPC version because it's easy to forget to keep them in sync.
This PR moves style-specific log from the
Datadog::Tracing::Distributednamespace toDatadog::Tracing::Contrib.This is simply a large refactor: there are no changes to externally visible behavior.
Tests remain the same, and passing, with coverage increased in a few cases.
Affected modules that are part of the public API,
Datadog::Tracing::Distributed::Headers::ExtandDatadog::Tracing::Propagation::HTTP, have a compatibility layer added to this PR to ensure there are no breaking changes.