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

opentelemetry: embed no-op structs #2284

Merged
merged 4 commits into from
Nov 10, 2023
Merged

Conversation

katiehockman
Copy link
Contributor

@katiehockman katiehockman commented Oct 20, 2023

What does this PR do?

Embeds the opentelemetry-go noop structs into the DD OTel API. AIT-8692

Motivation

This is a build requirement for the OTel API after open-telemetry/opentelemetry-go#4620, and our code fails to compile if we upgrade to the latest main without this fix.

See their package docs for more details.

We had three options that would have 3 different consequences when the upstream makes a breaking change:

  • embedded.TracerProvider - will break at compile-time
  • trace.TracerProvider - will build but panic
  • noop.TracerProvider - will build and any missing functions will be a no-op

The noop.TracerProvider sounded like the safest option for us, as it will give us time to support new functions in the API without needing to rush.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.

For Datadog employees:

  • If this PR touches code that handles credentials of any kind, such as Datadog API keys, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@katiehockman katiehockman requested a review from a team October 20, 2023 17:39
@katiehockman katiehockman requested a review from a team as a code owner October 20, 2023 17:39
@katiehockman
Copy link
Contributor Author

katiehockman commented Oct 20, 2023

Note that right now, the tests will fail due to compilation errors in the upstream when we make this change. All of our code builds, but when I try to run our tests, I get errors that look like this:

../../../../pkg/mod/go.opentelemetry.io/[email protected]/internal/global/trace.go:56:30: cannot use &tracerProvider{} (value of type *tracerProvider) as trace.TracerProvider value in variable declaration: *tracerProvider does not implement trace.TracerProvider (missing method tracerProvider)
../../../../pkg/mod/go.opentelemetry.io/[email protected]/internal/global/trace.go:104:10: cannot use val (variable of type *tracer) as trace.Tracer value in return statement: *tracer does not implement trace.Tracer (missing method tracer)
../../../../pkg/mod/go.opentelemetry.io/[email protected]/internal/global/trace.go:109:9: cannot use t (variable of type *tracer) as trace.Tracer value in return statement: *tracer does not implement trace.Tracer (missing method tracer)

It's unclear to me at first glance if this is something that needs to be fixed on the upstream, or on our side. I'll keep looking into it.

Update:
It's because I only upgraded go.opentelemetry.io/otel/trace and not go.opentelemetry.io/otel. Fixing that now. Commit incoming.

The tests should pass now.

@pr-commenter
Copy link

pr-commenter bot commented Oct 20, 2023

Benchmarks

Benchmark execution time: 2023-11-10 17:50:37

Comparing candidate commit 9d1bda3 in PR branch katie.hockman/otel-api-no-op with baseline commit 491dab0 in branch main.

Found 0 performance improvements and 2 performance regressions! Performance is the same for 37 metrics, 2 unstable metrics.

scenario:BenchmarkOTelApiWithCustomTags/otel_api-24

  • 🟥 allocated_mem [+92 bytes; +104 bytes] or [+2.381%; +2.687%]
  • 🟥 allocations [+1; +1] or [+3.030%; +3.030%]

Copy link
Contributor

@ajgajg1134 ajgajg1134 left a comment

Choose a reason for hiding this comment

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

Overall looks good to me! Maybe we should add a note to the go docs for users of the otel api that if they upgrade their underlying OTEL dependencies for new features before we've built support they will be a no-op?

ddtrace/opentelemetry/span.go Show resolved Hide resolved
@katiehockman
Copy link
Contributor Author

katiehockman commented Oct 24, 2023

We will wait to merge this until the upstream OTel repo has released their changes. At that time, I'll update the go.mod file to point to the latest published version, merge this, and we'll release ASAP after that (in a point release if needed).

The release is out: https://github.com/open-telemetry/opentelemetry-go/releases

@katiehockman katiehockman force-pushed the katie.hockman/otel-api-no-op branch from d1e0716 to 9d1bda3 Compare November 10, 2023 17:34
@katiehockman
Copy link
Contributor Author

katiehockman commented Nov 10, 2023

The parametric tests are failing now, but they'll work once DataDog/system-tests#1779 is merged.

Tests are now passing because DataDog/system-tests#1779 was merged 😄

@katiehockman katiehockman merged commit d264060 into main Nov 10, 2023
50 checks passed
@katiehockman katiehockman deleted the katie.hockman/otel-api-no-op branch November 10, 2023 20:48
@darccio darccio restored the katie.hockman/otel-api-no-op branch November 16, 2023 11:34
@darccio darccio deleted the katie.hockman/otel-api-no-op branch November 16, 2023 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants