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

[Trace] Mimimum changes in dskit in order to migrate to opentelemetry #385

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ying-jeanne
Copy link
Contributor

@ying-jeanne ying-jeanne commented Sep 21, 2023

What this PR does:

This PR showcases minimal code adjustments in dskit to transition from migrating opentracing to open-telemetry instrumentation. While this PR isn't meant for immediate merging, it serves as a basis for reviewing the changes and creating a design document if we want to maintain backward compatibility for team not ready for migration.

Key Inclusions in this PR:

  • Introduction of oteltracing.go: This file initializes the global tracerProvider. It preserves the Jaeger environment tracing settings, mainly concerning the following environment variables:
JAEGER_AGENT_HOST,JAEGER_TAGS,JAEGER_SAMPLER_MANAGER_HOST_PORT, JAEGER_SERVICE_NAME, JAEGER_SAMPLER_TYPE, and JAEGER_SAMPLER_PARAM

It also maintains compatibility for HTTP/grpc requests by encoding/decoding both opentracing (Jaeger trace header) and opentelemetry (W3C header).

  • Incorporation of spanlogger.go: This file combines a spanLogger object, which bundles a span and logger together. Many projects use this object, even if they don't employ dskit's tracing instrumentation. It exposes a public opentracing.Span accessible to libraries that import it.

  • grpcclient/instrumentation.go: This section addresses the different interceptors used by opentracing and opentelemetry.

  • use of otelhttp and otelhttptrace: These modules are used whenever there is a need to inject context into request headers or extract request headers into context. They replace the previous methods since they manage the injection and extraction of corresponding headers.

  • Utilization of opentelemetry's test tracerprovider with an in-memory exporter: All traces in unit tests have been adapted to use the tracetest tracerprovider provided by opentelemetry.

  • Modification in extracting the traceID from context: Instead of directly casting with jaeger.SpanContext from the context, this PR replaces it with otelSpan.SpanContext().TraceID().

This PR is being testing on dev environment.

The PR is generated by both gopatch and manually update, the gopatch file is located in https://gist.github.com/ying-jeanne/5e57b736bc8f3b1ab299928c54809eac

Which issue(s) this PR fixes:

Fixes #

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@ying-jeanne ying-jeanne changed the title [WIP] Mimimum changes in dskit in order to migrate to opentelemetry [Otel] Mimimum changes in dskit in order to migrate to opentelemetry Sep 21, 2023
@ying-jeanne ying-jeanne changed the title [Otel] Mimimum changes in dskit in order to migrate to opentelemetry [Trace] Mimimum changes in dskit in order to migrate to opentelemetry Sep 21, 2023
@ying-jeanne
Copy link
Contributor Author

The note for reviewer is above, and let me know if it doesn't appear clear.

@ying-jeanne ying-jeanne force-pushed the otel-migration-build-tag branch 2 times, most recently from 0afbbe7 to 3e456e1 Compare September 25, 2023 18:49
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.

1 participant