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

Add context propagation type to generateUUID method #784

Conversation

jhssilva
Copy link
Contributor

@jhssilva jhssilva commented Jan 29, 2025

What does this PR do?

  • Improves the method generateUUID by generating UUIDs for different Propagator type

Motivation

The current generateUUID method is incomplete as it doesn't generates correctly spans for RUM, or trace context propragation.

Additional Notes

Two approaches were consider:

  • Exposing the toString method allowing users to define the TracingIdFormat
  • Improve the generateUUID allowing users to choose the traceContext

Context
RUM resources requires (context):

  • TraceId: TracingIdFormat.paddedHex
  • SpanId: TracingIdFormat.decimal

While the Datadog Trace Context propagation requires (context):

  • TraceId: TracingIdFormat.lowDecimal
  • SpanId: TracingIdFormat.decimal

This creates a discrepancy when generating UUIDs for inclusion in headers and resources, particularly to circumvent cases where SSL pinning is used or when dealing with libraries not supported by the SDK.

Tests

For the tests it was mimic from the following code

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)
  • If this PR is auto-generated, please make sure also to manually update the code related to the change

@marco-saia-datadog marco-saia-datadog changed the title Add context propragation type to generateUUID method Add context propagation type to generateUUID method Jan 30, 2025
@jhssilva jhssilva marked this pull request as ready for review January 31, 2025 13:29
@jhssilva jhssilva requested a review from a team as a code owner January 31, 2025 13:29
@marco-saia-datadog
Copy link
Member

@jhssilva Amazing 🚀 thank you for the contribution!

I have implemented your changes in #793, since I wanted to encapsulate the logic in a separate class, since IMO it was becoming a bit too complex to keep it in DdRum.ts.

The idea is to return an object on which devs can call:

  • forResource() to format it for resources
  • forContextPropagation(propagationType) to format it for the desired context propagation type
  • toString(tracingIdFormat) to format it to a custom format (potentially covering future use cases we are not aware of)
  • type to verify the generated UUID type
  • id to access the UUID in raw format (big integer)

@marco-saia-datadog marco-saia-datadog added the duplicate This issue or pull request already exists label Feb 5, 2025
@marco-saia-datadog
Copy link
Member

Closing this PR as it is been replaced by #793

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants