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 OTLP and sampling to otelutils #5504

Merged
merged 2 commits into from
Jun 25, 2024
Merged

Add OTLP and sampling to otelutils #5504

merged 2 commits into from
Jun 25, 2024

Conversation

andrewwdye
Copy link
Contributor

Why are the changes needed?

This changes adds support for OLTP and sampling to the otelutils tracer provider abstraction.

Adds support for OLTP. This is the recommended replacement for the deprecated jaeger exporter.

"go.opentelemetry.io/otel/exporters/jaeger" is deprecated: This module is no longer supported. OpenTelemetry dropped support for Jaeger exporter in July 2023. Jaeger officially accepts and recommends using OTLP

OLTP supports grpc and http, which are added as separate exporter types and configs.

Adds initial sampling support to the top level open telemetry config. Defaults to parent sampler always, but also adds a config for TraceIdRatioBased. See these docs for behavior of parent sampler.

How was this patch tested?

Ran local sandbox with local jaeger all-in-one and verified

  • otlpgrpc with parent sampler always
  • otlpgrpc with parent sampler traceid (along with other defaults)
  • otlphttp with parent sampler traceid (along with other defaults)

Flyte config

❯ docker run --rm -e COLLECTOR_OTLP_ENABLED=true -p 16686:16686 -p 4317:4317 -p 4318:4318 jaegertracing/all-in-one:1.52

Jaeger

otel:
  type: otlpgrpc
  sampler:
    parentSampler: traceid

@andrewwdye andrewwdye force-pushed the andrew/otelutils-otlp branch from 9f77223 to 1b5eb0f Compare June 22, 2024 22:18
Copy link

codecov bot commented Jun 22, 2024

Codecov Report

Attention: Patch coverage is 45.94595% with 20 lines in your changes missing coverage. Please review.

Project coverage is 60.99%. Comparing base (2334d3f) to head (d504995).
Report is 128 commits behind head on master.

Files with missing lines Patch % Lines
flytestdlib/otelutils/factory.go 39.39% 20 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5504      +/-   ##
==========================================
- Coverage   61.01%   60.99%   -0.02%     
==========================================
  Files         794      794              
  Lines       51441    51470      +29     
==========================================
+ Hits        31385    31395      +10     
- Misses      17164    17183      +19     
  Partials     2892     2892              
Flag Coverage Δ
unittests-datacatalog 69.31% <ø> (ø)
unittests-flyteadmin 58.73% <ø> (ø)
unittests-flytecopilot 17.79% <ø> (ø)
unittests-flytectl 68.03% <ø> (ø)
unittests-flyteidl 79.04% <ø> (ø)
unittests-flyteplugins 61.85% <ø> (ø)
unittests-flytepropeller 57.30% <ø> (ø)
unittests-flytestdlib 65.59% <45.94%> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrewwdye andrewwdye requested a review from hamersaw June 22, 2024 22:24
@andrewwdye andrewwdye marked this pull request as ready for review June 22, 2024 22:24
@andrewwdye andrewwdye merged commit e1d9c5c into master Jun 25, 2024
51 of 53 checks passed
@andrewwdye andrewwdye deleted the andrew/otelutils-otlp branch June 25, 2024 14:44
robert-ulbrich-mercedes-benz pushed a commit to robert-ulbrich-mercedes-benz/flyte that referenced this pull request Jul 2, 2024
* Add OTLP and sampling to otelutils

Signed-off-by: Andrew Dye <[email protected]>

* Add RegisterTracerProviderWithContext

Signed-off-by: Andrew Dye <[email protected]>

---------

Signed-off-by: Andrew Dye <[email protected]>
vlibov pushed a commit to vlibov/flyte that referenced this pull request Aug 16, 2024
* Add OTLP and sampling to otelutils

Signed-off-by: Andrew Dye <[email protected]>

* Add RegisterTracerProviderWithContext

Signed-off-by: Andrew Dye <[email protected]>

---------

Signed-off-by: Andrew Dye <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
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.

3 participants