feat: add --disable-otel / DISABLE_OTEL to disable otel trace export#145
feat: add --disable-otel / DISABLE_OTEL to disable otel trace export#145galexrt wants to merge 1 commit intofivemanage:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a --disable-otel CLI flag and DISABLE_OTEL environment variable to allow users to opt out of OpenTelemetry trace export. This is intended to eliminate noisy connection-refused log messages when no OTEL collector is running at localhost:4318.
Changes:
pkg/otel/exporter.go: Adds adisabled boolparameter toSetupTracerandInstallExportPipeline; whendisabledis true, sets up a bare (no-exporter) tracer provider instead of connecting to the OTLP endpoint.cmd/lite/lite.go: Registers the--disable-otelflag and binds it to theDISABLE_OTELenv var via viper; passes the value intootel.SetupTracer.README.md/.env.template: Documents the newDISABLE_OTELconfiguration option.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pkg/otel/exporter.go |
Core logic change: skips OTLP exporter setup when disabled=true |
cmd/lite/lite.go |
Registers --disable-otel flag and wires it to viper/env |
README.md |
Adds DISABLE_OTEL to the configuration table |
.env.template |
Adds DISABLE_OTEL=false to the dev template |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rootCmd.Flags().String("s3-provider", "minio", "S3 provider") | ||
| rootCmd.Flags().String("bucket-domain", "", "Bucket domain for file storage") | ||
| // otel | ||
| rootCmd.Flags().Bool("disable-otel", false, "Disable OpenTelemetry") |
There was a problem hiding this comment.
The --disable-otel CLI flag is registered with rootCmd.Flags().Bool(...) but there is no viper.BindPFlag("disable-otel", rootCmd.Flags().Lookup("disable-otel")) call, unlike other flags such as port and dsn. Without this binding, viper.GetBool("disable-otel") will never read the value supplied via the command-line flag — it will only pick up the value from the DISABLE_OTEL environment variable. To make the flag functional, a corresponding viper.BindPFlag call must be added alongside the existing viper.BindEnv call.
pkg/otel/exporter.go
Outdated
| func InstallExportPipeline(ctx context.Context) (func(context.Context) error, error) { | ||
| func InstallExportPipeline(ctx context.Context, disabled bool) (func(context.Context) error, error) { | ||
| if disabled { | ||
| tracerProvider := trace.NewTracerProvider() |
There was a problem hiding this comment.
When OTEL is disabled, the code creates a full SDK trace.NewTracerProvider() with no exporter but with the default sampler (ParentBased(AlwaysSample)). This means spans are still allocated in memory throughout the application — they are just never exported. A more efficient approach would be to use otel.NewNoopTracerProvider() (from go.opentelemetry.io/otel) instead, which provides a true no-op implementation that skips span creation entirely and incurs zero overhead.
| tracerProvider := trace.NewTracerProvider() | |
| tracerProvider := otel.NewNoopTracerProvider() |
e42d94b to
e71e694
Compare
Signed-off-by: Alexander Trost <galexrt@googlemail.com>
e71e694 to
5fc55f5
Compare
Add flag/env var to allow disabling OTEL export/traces.
This should eliminate logs such as this when there isn't OTEL export available at
localhost:4318:Are there any steps I'm missing to build the project? I can't seem to build it due to "docs" pkg missing:
Looking around, the
web/package.jsontargetgenerate:apiaccesses thedocs/dir as well, but there is nodocs/dir after following the steps from theREADME.md.I would appreciate any hints to get it to build locally so I can test it and also look into fixing another issue (nil panics when clickhouse is disabled/not used).