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

✨ [RUMF-1236] Add support for OTel headers #1832

Merged
merged 5 commits into from
Dec 15, 2022

Conversation

yannickadam
Copy link
Contributor

@yannickadam yannickadam commented Nov 21, 2022

Motivation

Users would like to use RUM alongside backends that are instrumented with OpenTelemetry, and benefit from the correlation of traces in APM.

Changes

Added a new RUM configuration property:
allowedTracingUrls: { match: MatchOption, propagatorTypes: ("datadog" | "tracecontext" | "b3" | "b3multi")[] }

  • datadog: Datadog (x-datadog-*)
  • tracecontext: Trace Context (traceparent)
  • b3: B3 Single Header (b3)
  • b3multi: B3 Multi Headers (X-B3-*)

This allows to select which propagator types to use when creating the request headers. It also gives more flexibility by providing the full URL to the match function (instead of origin).

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@yannickadam yannickadam requested a review from a team as a code owner November 21, 2022 13:54
@yannickadam yannickadam marked this pull request as draft November 21, 2022 16:02
@yannickadam yannickadam force-pushed the yannick.adam/rumf-1236-otel-headers branch from c3a2f42 to 2f8a97d Compare December 6, 2022 23:23
@yannickadam yannickadam changed the title ✨ Add support for OTel headers ✨ [RUMF-1236] Add support for OTel headers Dec 6, 2022
@yannickadam yannickadam marked this pull request as ready for review December 6, 2022 23:27
@yannickadam yannickadam force-pushed the yannick.adam/rumf-1236-otel-headers branch from 2f8a97d to 43dddef Compare December 7, 2022 08:58
@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2022

Codecov Report

Merging #1832 (dc7d21d) into main (0fe202a) will increase coverage by 0.01%.
The diff coverage is 95.28%.

@@            Coverage Diff             @@
##             main    #1832      +/-   ##
==========================================
+ Coverage   93.06%   93.07%   +0.01%     
==========================================
  Files         132      132              
  Lines        5103     5185      +82     
  Branches     1143     1173      +30     
==========================================
+ Hits         4749     4826      +77     
- Misses        354      359       +5     
Impacted Files Coverage Δ
packages/rum-core/src/domain/configuration.ts 93.61% <91.22%> (-4.17%) ⬇️
packages/core/src/browser/cookie.ts 86.48% <100.00%> (ø)
packages/core/src/tools/utils.ts 82.94% <100.00%> (+0.26%) ⬆️
packages/rum-core/src/domain/tracing/tracer.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@yannickadam yannickadam force-pushed the yannick.adam/rumf-1236-otel-headers branch 2 times, most recently from 427e90a to 896b641 Compare December 7, 2022 13:02
@yannickadam yannickadam force-pushed the yannick.adam/rumf-1236-otel-headers branch from 896b641 to 95c77f0 Compare December 7, 2022 13:20
@liywjl liywjl self-requested a review December 8, 2022 09:43
packages/rum-core/src/domain/configuration.spec.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/configuration.spec.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/configuration.spec.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/configuration.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/configuration.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/tracing/tracer.spec.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/tracing/tracer.spec.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/tracing/tracer.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/tracing/tracer.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/configuration.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/tracing/tracer.types.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/tracing/tracer.types.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/tracing/tracer.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/tracing/tracer.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/tracing/tracer.ts Outdated Show resolved Hide resolved
packages/rum-core/src/domain/configuration.ts Outdated Show resolved Hide resolved
@zacharycmontoya
Copy link

  • b3m: B3 Multi Headers (X-B3-*)

If we want to align this with OpenTelemetry conventions, we should name this b3multi, see https://opentelemetry.io/docs/reference/specification/sdk-environment-variables/

@zacharycmontoya
Copy link

Also, the OTEL convention is the name tracecontext to represent the traceparent/tracestate headers, so it would be great to match that too

@yannickadam yannickadam force-pushed the yannick.adam/rumf-1236-otel-headers branch 2 times, most recently from dc7d21d to a517875 Compare December 12, 2022 07:41
Copy link
Member

@BenoitZugmeyer BenoitZugmeyer left a comment

Choose a reason for hiding this comment

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

This PR updates rum-events-format, but I don't think that was intended, maybe revert it

Comment on lines +147 to +149
propagatorTypes.forEach((propagatorType) => {
switch (propagatorType) {
case 'datadog': {
Copy link
Member

Choose a reason for hiding this comment

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

👏 praise: ‏It looks great, nice job!

@yannickadam yannickadam force-pushed the yannick.adam/rumf-1236-otel-headers branch from a517875 to 7c2c512 Compare December 12, 2022 13:46
Copy link

@zacharycmontoya zacharycmontoya left a comment

Choose a reason for hiding this comment

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

From an APM Tracer dev that's working on OTEL headers, this looks good to me 👍🏼

Comment on lines +676 to +677
try {
if (typeof item === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

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

💬 suggestion: ‏if you had some error cases in mind when expanding the scope of the try/catch, it could be nice to add some tests for them

@@ -36,7 +36,7 @@ export function areCookiesAuthorized(options: CookieOptions): boolean {
// the test cookie lifetime
const testCookieName = `dd_cookie_test_${generateUUID()}`
const testCookieValue = 'test'
setCookie(testCookieName, testCookieValue, ONE_SECOND, options)
setCookie(testCookieName, testCookieValue, ONE_MINUTE, options)
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ question: I am curious why this needed changing?

@yannickadam yannickadam merged commit 83b2be6 into main Dec 15, 2022
@yannickadam yannickadam deleted the yannick.adam/rumf-1236-otel-headers branch December 15, 2022 16:01
@naseemkullah
Copy link

great job!

when will this be released? looking to use this asap cc @BenoitZugmeyer @liywjl

@bcaudan
Copy link
Contributor

bcaudan commented Dec 29, 2022

Hi @naseemkullah, it should be released early January.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants