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

tracer: merge support for OTel API #1839

Merged
merged 16 commits into from
Apr 4, 2023
Merged

tracer: merge support for OTel API #1839

merged 16 commits into from
Apr 4, 2023

Conversation

dianashevchenko
Copy link
Contributor

@dianashevchenko dianashevchenko commented Mar 29, 2023

What does this PR do?

This PR adds support for an OpenTelemetry compatible API which wraps the Datadog API.

Motivation

This can make it easier for dd-trace-go users who have already instrumented their code with OpenTelemetry and want to utilize the features of the dd-trace-go library with as few code changes as possible. Migrating from using the upstream OTel SDK (or another OTel API implementation) to using this Datadog OTel API should be as simple as subbing out their existing TracerProvider for ours.

Describe how to test/QA your changes

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

dianashevchenko and others added 9 commits March 7, 2023 15:03
Added an OTel API skeleton & span methods
* added an otel api skeleton & some span methods

* lint & copyright

* fixed lint

* updated comments

* updated span operations

* updated tracer / tracerProvider

* fixed lint & copyright

* fixed lint

* updated according to comments

* updated tests + add simple benchmark

* fixed lint & copyright

* removed a comment

* Add BenchmarkOtelConcurrentTracing

* added span testing function stubs

* added WIP unit tests for TestForceFlush, TestShutdown

* fixed TestForceshutdown log error

* refactored span method testing, refined otel tracer test fns

* added TestForceFlush

* commented use of waitForTestAgent

* added env var testing

* remove test case

* added unit test for SpanContext

* Update ddtrace/opentelemetry/span_test.go

Co-authored-by: Diana Shevchenko <40775148+dianashevchenko@users.noreply.github.com>

* Update ddtrace/opentelemetry/span_test.go

Co-authored-by: Diana Shevchenko <40775148+dianashevchenko@users.noreply.github.com>

* refactored test cases

* removed new lines

* fixed blocking test cases

* updated test agent

* Update ddtrace/opentelemetry/span_test.go

Co-authored-by: Diana Shevchenko <40775148+dianashevchenko@users.noreply.github.com>

* Update ddtrace/opentelemetry/span_test.go

Co-authored-by: Diana Shevchenko <40775148+dianashevchenko@users.noreply.github.com>

* unmarshal payload

* refactor test tracer provider

* fix lint: move ctx.Context to first argument for waitForPayload

* lint

* unit test for test tracer start options

* fix flaky TestSpanWithContext

* fix flaky agent timeout

* refactored waitForPayload

* flip success bool forceflush

* update go.mod

* Update ddtrace/opentelemetry/tracer_test.go

Co-authored-by: Katie Hockman <katie@hockman.dev>

* Update ddtrace/opentelemetry/span_test.go

Co-authored-by: Katie Hockman <katie@hockman.dev>

* cleanup/refactor testing code according to comments

* use iota for flushstatus constants

* refactor span set status test

* fix typos

* put test case for span end inline

* naming consistency for span end test

* prevent flaky forceflush test

* use null logger

* remove null logger

---------

Co-authored-by: Diana Shevchenko <diana.shevchenko@datadoghq.com>
Co-authored-by: Diana Shevchenko <40775148+dianashevchenko@users.noreply.github.com>
Co-authored-by: Katie Hockman <katie@hockman.dev>
@pr-commenter
Copy link

pr-commenter bot commented Mar 29, 2023

Benchmarks

Comparing candidate commit fb0c2dd in PR branch shared/otel-api with baseline commit ae6eed9 in branch main.

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

scenario:BenchmarkConcurrentTracing-24

  • 🟥 execution_time [+0.172ms; +0.186ms] or [+14.384%; +15.587%]

@ahmed-mez
Copy link
Contributor

We should revert #1849 as part of this PR.

@dianashevchenko dianashevchenko marked this pull request as ready for review March 31, 2023 13:41
@dianashevchenko dianashevchenko requested a review from a team March 31, 2023 13:41
@dianashevchenko dianashevchenko requested a review from a team as a code owner March 31, 2023 13:41
@dianashevchenko
Copy link
Contributor Author

We have run locally the parametric tests with all the tests unskipped

@ahmed-mez
Copy link
Contributor

But we still want to tests to run on main as well right?

@dianashevchenko
Copy link
Contributor Author

Sure, but for that we have to remove the fix and unskip the tests in parametric, introducing a blocker again. So for now, we're running tests manually

@ahmed-mez
Copy link
Contributor

Unskipping the tests won't break main since we're pinning an older version of system-tests that doesn't contain otel tests at all

@dianashevchenko
Copy link
Contributor Author

We have run locally the parametric tests with all the tests unskipped

This comment is not related to a previous comment of Ahmed.

@ahmed-mez
Copy link
Contributor

Sorry for the confusion lol!

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.

Looks good to me!

@katiehockman katiehockman changed the title Merge support for OTel API tracer: merge support for OTel API Apr 4, 2023
@katiehockman katiehockman merged commit 11baded into main Apr 4, 2023
@katiehockman katiehockman deleted the shared/otel-api branch April 4, 2023 20:08
eliottness pushed a commit that referenced this pull request Apr 5, 2023
Adds support for an OpenTelemetry compatible API which wraps the Datadog API.
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.

5 participants