[w3c] Support W3C trace context propagation #429
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aiming to enhance its use, created for xray trace context handling, now we expect it to use it for all of the tracing context
simple wrapper to use instead of `TraceContext`, this allows us to type the object extracted by the tracer. I decided to wrap it into a class, to easily managea black-box class – yes, I know we are importing it in one of the constructors, but that is the easiest way to create one for trace context with Xray, StepFunction event, and for Custom Extractors, so we can tell the tracer what is a child of what.
make it complaint to use the refactored classes, also updated its unit tests to mock the new methods being used
the main point of this class is to create a singleton state manager for the `StepFunctionContext` interface, it will run per invocation, and should not interfere with new invocations
this new class handles all logic related to Xray, previously, it was scattered in `context.ts`, but it actually didnt make sense, since we had multiple functions which should be in the same place, we could also keep a state since we are relying on a header and context which will always be deterministic, so no need to calculate it again in multiple functions
this file will be in charge of extracting the trace context from the incoming context, event, step function context, or xray, separating the logic into an adapter pattern for a much simpler and easier way of handling this extraction
refactoring code to make sure we correctly handling an HTTP event for trace context
allow custom extractor to be in a separate class, which then uses a helper method to generate an `SpanContextWrapper` to be returned, had to deprecate some fields, so to avoid breaking changes, I have marked some fields as nullable
basically relies on `http.ts` to work
same as other extractors, separation of concerns into its own file
same as other extractors, separation of concerns into its own file
same as other extractors, separation of concerns into its own file
same as other extractors, separation of concerns into its own file, I think we could do some more refactoring, but for now, I will leave sns-sqs and eventbridge-sqs as separate files
same as other extractors, separation of concerns into its own file
same as other extractors, separation of concerns into its own file, related to sns-sqs comment, maybe in the future we could explore refactoring this extraction, but for now, will stay as is
same as other extractors, separation of concerns into its own file, also made sure legacy payloads still work
same as other extractors, separation of concerns into its own file, uses the singleton class to get context
basically to export them from the root
updated unit tests to make sure everything works as before with the new methods and mockings
w3c will not be supported here, since we would have to do manual extraction and injection, this will not be supported for customers not using `dd-trace`, so the refactoring is just to use the new methods and mockings
w3c will not be supported here, since we would have to do manual extraction and injection, this will not be supported for customers not using `dd-trace`, so the refactoring is just to use the new methods and mockings, if a new user wants to correlate traces with logs without `dd-trace`, they will have to use the datadog headers, this might be up for removal in the future
make use of the new services, also refactored unit tests to work as expected
might need to re-explore this method, but for safety, I am adding it just to make sure we are indeed creating a new instance of the service on every invocation
export from the right file
this class aims to replace `event-type-guards.ts`, I think the file name is confusing, and we should use a class as a namespace here, nonetheless, I wonder if this change is necessary at all, since it would mostly use the same exact logic, but allowing it to be exported as a single class with static methods, this is exactly the same as what we do when importing the whole file.
decomissioning a large file and its unit tests, made sure to refactor every single method into multiple files and classes
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #429 +/- ##
==========================================
+ Coverage 81.19% 81.96% +0.76%
==========================================
Files 50 54 +4
Lines 2042 2179 +137
Branches 467 506 +39
==========================================
+ Hits 1658 1786 +128
- Misses 323 330 +7
- Partials 61 63 +2 ☔ View full report in Codecov by Sentry. |
|
It's normal for integration tests to not pass, since now, console patching only happens when -- edit: fixed |
…ing the `SpanContextWrapper` class as usual
…erate the context everytime, I tried to remove this by having the `header` and `context` as properties, but somehow, it ends up being `undefined`, therefore the switch back
11 tasks
duncanista
changed the base branch from
main
to
jordan.gonzalez/SVLS-3939/re-organize-trace-context-extraction
duncanista
changed the base branch from
jordan.gonzalez/SVLS-3939/re-organize-trace-context-extraction
to
main
astuyve
previously requested changes
Nov 13, 2023
astuyve
reviewed
Nov 13, 2023
astuyve
approved these changes
Nov 20, 2023
purple4reina
approved these changes
Nov 20, 2023
astuyve
deleted the
jordan.gonzalez/SVLS-3939/support-w3c-trace-context-headers
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
TLDR; Refactor code and delegate trace extraction to the tracer.
This PR does a couple things:
Motivation
Our library was extracting tracing context only for Datadog headers, the need to extract from W3C trace context is expected and already being done by the
dd-tracepackage. Therefore, we should delegate our previous manual extraction to the already predefined functions.Testing Guidelines
Services using W3C context in headers in AWS Lambda.
Additional Notes
consolewhen no tracer is available will fallback to always propagate Datadog Headers only.traceIDandparentIDin the custom extractor expected value is in place, in the next breaking change, this will be removed in favor oftraceIdandparentId.Types of Changes
Check all that apply