feat: Add OTEL default variables #1812
Conversation
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
|
@askpt I would suggest using https://pkg.go.dev/go.opentelemetry.io/contrib/exporters/autoexport and not re-implement the entire otel sdk. |
Thank you so much for the suggestion @erka! I was not aware of this library, and I will take a look. I am concerned that it may not be easily switched, as we need to maintain backwards compatibility with the previous settings. |
|
You can likely configure the OTEL environment variables within the code - using the flagd configuration - prior to initializing autoexport to maintain backwards compatibility. |
…erride Signed-off-by: André Silva <[email protected]>
…ation Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
…sistency Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
…uration. Signed-off-by: André Silva <[email protected]>
askpt
force-pushed
the
askpt/1141-add-otel-variables
branch
from
94a88af to
6d015b6
Compare
dosubot
bot
added
the
size:L
|
@askpt I've resolved the FOSSA issues. |
Signed-off-by: André Silva <[email protected]>
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant improvement, refactoring the OpenTelemetry integration to use the autoexport package. This change greatly simplifies the codebase by removing custom logic for OTLP exporter setup and transport credentials, instead relying on standard environment variable-based configuration. The addition of backward compatibility for existing flag-based configurations by setting the corresponding environment variables is a well-thought-out approach. The overall result is a cleaner, more maintainable, and more standard-compliant telemetry setup. The dependency updates and test adjustments are also appropriate and align with the refactoring. Great work!
|
|
||
| // buildDefaultMetricReader provides the default metric reader | ||
| func buildDefaultMetricReader() (metric.Reader, error) { | ||
| func buildDefaultMetricReader(ctx context.Context) (metric.Reader, error) { |
There was a problem hiding this comment.
The ctx parameter in buildDefaultMetricReader is not used. It's good practice to name unused parameters with an underscore (_) to make it explicit to readers of the code that it is intentionally unused.
| func buildDefaultMetricReader(ctx context.Context) (metric.Reader, error) { | |
| func buildDefaultMetricReader(_ context.Context) (metric.Reader, error) { |
| // Set OTEL environment variables from configuration if not already set | ||
| setEnvIfNotSet("OTEL_METRICS_EXPORTER", "otlp") | ||
| setEnvIfNotSet("OTEL_EXPORTER_OTLP_ENDPOINT", cfg.CollectorConfig.Target) | ||
| setEnvIfNotSet("OTEL_EXPORTER_OTLP_PROTOCOL", "grpc") |
There was a problem hiding this comment.
Is gRPC the right default here? Or should we add other options. I have NO idea myself, so I will defer to you on this one.
cc @beeme1mr
https://opentelemetry.io/docs/languages/sdk-configuration/otlp-exporter/#otel_exporter_otlp_protocol
There was a problem hiding this comment.
Ya... based on my research, I think "http/protobuf" might be a safer default.
There was a problem hiding this comment.
It was used by flagd before. I believe it's a fallback value for backward compatibility.
|
It seems right to me, but I'm far from being an OTel expert, especially WRT to the |
4 participants
Signed-off-by: André Silva [email protected]
This PR
This pull request updates the OpenTelemetry integration in the
coremodule to use the newautoexportpackage, simplifying configuration and improving compatibility with environment variables. It removes custom gRPC transport credential handling and centralizes exporter setup, making telemetry easier to configure and maintain. Several dependencies are updated to their latest versions, and tests are adjusted to reflect the new configuration logic.OpenTelemetry Integration Refactor:
autoexportpackage, removing custom TLS handling and simplifying trace and metric exporter initialization incore/pkg/telemetry/builder.go. Now, environment variables are used for configuration, and Prometheus is used as a fallback for metrics. [1] [2] [3] [4]core/pkg/telemetry/builder_test.goto reflect the new autoexport logic, removing tests for unsupported configurations and ensuring the interceptor is always added. [1] [2]Dependency Updates:
core/go.mod, includingprometheus/client_golang,stretchr/testify,go.opentelemetry.io/otel, and related packages to their latest versions. Added new dependencies forautoexportand related exporters.github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp,github.com/cenkalti/backoff/v5,github.com/go-jose/go-jose/v4, and others. [1] [2] [3] [4] [5]These changes make telemetry setup more robust and future-proof, reducing custom code and leveraging upstream improvements.
Related Issues
Fixes #1141
Notes