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

RUM-1836 feat(otel-tracer): support multi level tags using key flattening #1626

Merged

Conversation

ganeshnj
Copy link
Contributor

What and why?

Currently, attributes that are array or set which is composition of primitive types are not supported.

How?

Attributes are flattened using the recursive flattening.

This is inspired from Java version of tracer https://github.com/DataDog/dd-trace-java/blob/0f986a71765d0bad85ed4e5c3bc6ef44b8cb78fe/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/trace/OtelSpan.java#L51-L63

The idea here is to create a key along with the path and save the value as it is in a flat dictionary (tags). In case of arrays, items are indexed with key.{index} pattern.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests for Core, RUM, Trace, Logs, CR and WVT
  • Run unit tests for Session Replay
  • Run integration tests
  • Run smoke tests
  • Run tests for tools/

@ganeshnj ganeshnj marked this pull request as ready for review January 10, 2024 17:31
@ganeshnj ganeshnj requested review from a team as code owners January 10, 2024 17:31
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Looks fine and its super clear 👌. I'm thinking why such significant change to the format of tags didn't require update in any tests added earlier - perhaps we could be missing integration coverage, no?

What would you say for adding an integration-unit test for this concept (from setting some tags to seeing them in encoded event)?

@ganeshnj
Copy link
Contributor Author

Looks fine and its super clear 👌. I'm thinking why such significant change to the format of tags didn't require update in any tests added earlier - perhaps we could be missing integration coverage, no?

What would you say for adding an integration-unit test for this concept (from setting some tags to seeing them in encoded event)?

That's because we are not working with internal of DatadogTracer, we know [String: String] works perfect and well tested.

Currently not needed, but when we start to deprecated OTTracer, yes then we need to migrate those test cases.

@ganeshnj ganeshnj merged commit 995818d into ganeshnj/feat/otel-tracer Jan 11, 2024
8 checks passed
@ganeshnj ganeshnj deleted the ganeshnj/feat/otel-tracer-complex-tags branch January 11, 2024 10:17
@ncreated ncreated mentioned this pull request May 31, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants