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

pkg/trace/api: add support for OpenTelemetry Span Links. #15767

Merged
merged 12 commits into from
Mar 15, 2023

Conversation

dinooliva
Copy link
Contributor

@dinooliva dinooliva commented Feb 23, 2023

What does this PR do?

Adds support for OpenTelemetry span links to the Trace Agent.

During the conversion from OTLP to the Datadog trace protobuf, any span with span links will have the span links added to the metadata with the key '_dd.span_links'.

Motivation

This change is part of an ongoing effort to support OpenTelemetry Span Links at Datadog.

Additional Notes

Span links will be JSON encoded as:
{ "trace_id":trace_id, "span_id":span_id[,"trace_state":trace_state]
[,"attributes":{ (key:value)+ }] [,"dropped_attributes_count":uint64] }

where trace_id & span_id are hex-encoded unsigned ints, trace_state is the W3C TraceState encoding as a string, and key & value are arbitrary strings.

Note that "trace_state", "attributes", and "dropped_attributes_count" are optional and will only be present if they contain non-empty/non-zero values.

Possible Drawbacks / Trade-offs

This change only affects the OTel path and only for spans that contain span links so it will only impact a small percentage of spans. The main possible drawback will be the added processing overhead (converting span links to JSON) and the added metadata.

Describe how to test/QA your changes

Send a span with span links from an OpenTelemetry service and check the resulting converted span has the links in the metadata.

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Use the major_change label if your change either has a major impact on the code base, is impacting multiple teams or is changing important well-established internals of the Agent. This label will be use during QA to make sure each team pay extra attention to the changed behavior. For any customer facing change use a releasenote.
  • A release note has been added or the changelog/no-changelog label has been applied.
  • Changed code has automated tests for its functionality.
  • Adequate QA/testing plan information is provided if the qa/skip-qa label is not applied.
  • At least one team/.. label has been applied, indicating the team(s) that should QA this change.
  • If applicable, docs team has been notified or an issue has been opened on the documentation repo.
  • If applicable, the need-change/operator and need-change/helm labels have been applied.
  • If applicable, the k8s/<min-version> label, indicating the lowest Kubernetes version compatible with this feature.
  • If applicable, the config template has been updated.

@dinooliva dinooliva requested review from a team as code owners February 23, 2023 17:44
@dinooliva dinooliva requested a review from dineshg13 February 23, 2023 17:44
@bits-bot
Copy link
Collaborator

bits-bot commented Feb 23, 2023

CLA assistant check
All committers have signed the CLA.

@dinooliva dinooliva added the team/opentelemetry OpenTelemetry team label Feb 23, 2023
@dinooliva dinooliva added this to the 7.45.0 milestone Feb 24, 2023
@dinooliva dinooliva requested a review from a team as a code owner February 24, 2023 18:03
@dinooliva dinooliva changed the title [WIP] Add support for OpenTelemetry Span Links. Add support for OpenTelemetry Span Links. Feb 25, 2023
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@dineshg13 dineshg13 left a comment

Choose a reason for hiding this comment

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

LGTM

@dinooliva
Copy link
Contributor Author

As requested by @dineshg13 , attaching a screenshot of the span link encoding from a test case.

Screen Shot 2023-03-02 at 11 33 34 AM

@dinooliva dinooliva requested a review from gbbr March 3, 2023 16:10
@gbbr gbbr changed the title Add support for OpenTelemetry Span Links. pkg/trace/api: add support for OpenTelemetry Span Links. Mar 6, 2023
pkg/trace/api/otlp.go Show resolved Hide resolved
pkg/trace/api/otlp.go Outdated Show resolved Hide resolved
pkg/trace/testutil/otlp.go Show resolved Hide resolved
pkg/trace/testutil/otlp.go Outdated Show resolved Hide resolved
pkg/trace/testutil/otlp.go Outdated Show resolved Hide resolved
pkg/trace/api/otlp.go Show resolved Hide resolved
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

I'm pre-approving because I don't feel strongly about my final comments and to unblock you from merging after potentially addressing them.

pkg/trace/testutil/otlp.go Show resolved Hide resolved
@@ -1274,6 +1397,138 @@ func trimSpaces(str string) string {
return out.String()
}

func makeSpanLinkSlice(traceId, spanId, traceState string, attrs map[string]string, dropped uint32) ptrace.SpanLinkSlice {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I usually think it's a good practice to test helper functions too. If it's a test helper, it doesn't mean it is reliable to work correctly 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was on the fence about this one since, if there were any problems with it, then TestMarshalSpanLinks would likely fail but it certainly doesn't hurt to add some basic testing.

Done.

pkg/trace/api/otlp_test.go Outdated Show resolved Hide resolved
@dinooliva dinooliva merged commit bedd4d0 into main Mar 15, 2023
@dinooliva dinooliva deleted the dino.oliva/span-links-support branch March 15, 2023 14:37
mackjmr added a commit to mackjmr/opentelemetry-collector-contrib that referenced this pull request Mar 23, 2023
…to stable.

This PR graduates feature gate `exporter.datadog.hostname.preview` to Stable. This is a follow up to [PR1](DataDog/opentelemetry-mapping-go#21) and [PR2](DataDog/datadog-agent#16159).

**Note:** This PR pins a pseudoversion of `github.com/DataDog/datadog-agent/pkg/trace` in order to use take advantage of changes ([commit](DataDog/datadog-agent@8b27e87)) done in preparation of this PR. This pseudoversion also contains a change to add support for span links ([PR](DataDog/datadog-agent#15767)), which has been added to the changelog.
mx-psi added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Mar 23, 2023
…to stable. (#20286)

* exporter/datadogexporter: Graduate exporter.datadog.hostname.preview to stable.

This PR graduates feature gate `exporter.datadog.hostname.preview` to Stable. This is a follow up to [PR1](DataDog/opentelemetry-mapping-go#21) and [PR2](DataDog/datadog-agent#16159).

**Note:** This PR pins a pseudoversion of `github.com/DataDog/datadog-agent/pkg/trace` in order to use take advantage of changes ([commit](DataDog/datadog-agent@8b27e87)) done in preparation of this PR. This pseudoversion also contains a change to add support for span links ([PR](DataDog/datadog-agent#15767)), which has been added to the changelog.

* add PR # to changelog

* run make gotidy

* Update .chloggen/datadog-exporter-span-links-support.yaml

Co-authored-by: Pablo Baeyens <[email protected]>

---------

Co-authored-by: Pablo Baeyens <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team/opentelemetry OpenTelemetry team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants