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

Make OTLP initialization more generic and add exporters #3

Merged
merged 2 commits into from Jan 27, 2022

Conversation

cjs
Copy link
Contributor

@cjs cjs commented Jan 26, 2022

In reference to authzed/spicedb#14, this enables the OpenTelemetry OTLP/HTTP and OTLP/GRPC exporters. Since just about all of the code except for creating the exporter was common with the existing Jaeger implementation, we abstracted out the tracer initiation.

We wanted to get some early feedback on this before it went further, as we had some choices to make. We wanted to make the new exporters work, but also wanted to have the most flexibility and not change this external library too much.

Some points to consider:

  1. The command line flags were specific to jaeger, but since they were common they were renamed to otel-endpoint and otel-service-name.
  2. We kept the previous method of configuring the jaeger exporter via command line flags for backward compatibility. However, since there are extensive environment variables provided by the OpenTelemetry specification and the Go SDK to configure the operation of the SDK, we thought it would be more flexible to bypass the command line flag for configuring the endpoints in favor of setting OTEL_EXPORTER_OTLP_ENDPOINT or OTEL_EXPORTER_OTLP_TRACES_ENDPOINT.
  3. The OTEL_JAEGER_EXPORTER_ENDPOINT could be used instead of a command line flag for the Jaeger exporter, which would require removing the Must checks.
  4. The variable OTEL_SERVICE_NAME is never used, since the service name is either set on the command line, or derived from the runtime.
  5. Would there be value in inlining the OpenTelemetry initialization into spiceDB?

Since this is a standalone library, we can change what we've done to keep the interfaces somewhat consistent.

Co-authored-by: Christina Troger <christroger@github.com>
cobrautil.go Outdated Show resolved Hide resolved
Co-authored-by: Christina Troger <christroger@github.com>
@cjs cjs marked this pull request as ready for review January 27, 2022 16:04
Copy link
Owner

@jzelinskie jzelinskie left a comment

Choose a reason for hiding this comment

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

Thanks for a ton for this.

We've spent very little time when it's come to taking fully advantage of OpenTelemetry, so it's great to see more use cases to flesh out the support.

@jzelinskie jzelinskie merged commit 203b454 into jzelinskie:main Jan 27, 2022
@cjs cjs deleted the hack-otlp branch February 9, 2022 17:27
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.

None yet

3 participants