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 OTel endpoint optional and use defaults if note set #4

Merged
merged 5 commits into from Feb 1, 2022

Conversation

cjs
Copy link
Contributor

@cjs cjs commented Jan 27, 2022

What

This change makes specifying an otel-endpoint optional. If not set, the OpenTelemetry exporters will be configured through:

  • The OpenTelementry environment variables, if set
  • The OpenTelemetry specification defaults

See https://github.com/open-telemetry/opentelemetry-go/tree/main/exporters/otlp/otlptrace#environment-variables and https://github.com/open-telemetry/opentelemetry-go/tree/main/exporters/jaeger#environment-variables for the defaults

Why

Setting the client with WithEndpoint forces the client to use the default URL Path. By making setting the endpoint via command line flag optional, we can unify the jaeger and otlp* provider command line flags, and still give the option of custom configurations via environment variables.

The alternative would be to add another flag for URLPath.

/cc authzed/spicedb#14
/cc @christroger

cobrautil.go Show resolved Hide resolved
cobrautil.go Outdated
if endpoint != "" {
exporter, err = jaeger.New(jaeger.WithCollectorEndpoint(jaeger.WithEndpoint(endpoint)))
} else {
exporter, err = jaeger.New(jaeger.WithCollectorEndpoint())
}
Copy link
Owner

@jzelinskie jzelinskie Jan 29, 2022

Choose a reason for hiding this comment

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

This seems fine for now, but in the future when there could be more options, it might make more sense to do something like

var opts []jaeger.CollectorEndpointOption
if endpoint != "" {
    opts = append(opts, jaeger.WithEndpoint(endpoint))
}
exporter, err := jaeger.New(opts...)

This strategy can likely be applied to all of the other providers, too.

Copy link
Contributor Author

@cjs cjs Jan 31, 2022

Choose a reason for hiding this comment

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

addressed in c4a5fd9

@christroger
Copy link
Contributor

@christroger christroger commented Jan 31, 2022

The additional changes I made, will allow us to set the Propagator to b3, ottrace and w3c. It is also possible to set multiple Propagators. To do that, the Propagators need to be separated by a comma. For example: --otel-trace-propagators "w3c,b3"
The default behaviour will use the w3c Propagators you already specified before.

cobrautil.go Outdated
@@ -173,28 +176,52 @@ func OpenTelemetryRunE(flagPrefix string, prerunLevel zerolog.Level) CobraRunFun

provider := strings.ToLower(MustGetString(cmd, prefixed("provider")))
serviceName := MustGetString(cmd, prefixed("service-name"))
endpoint, _ := cmd.Flags().GetString(prefixed("endpoint"))
Copy link
Owner

@jzelinskie jzelinskie Jan 31, 2022

Choose a reason for hiding this comment

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

This should probably be MustGetString(cmd, prefixed("endpoint"))

Copy link
Contributor

@christroger christroger Feb 1, 2022

Choose a reason for hiding this comment

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

True, updated it so the function uses the same Method to get the Flag value.

Copy link
Contributor Author

@cjs cjs Feb 2, 2022

Choose a reason for hiding this comment

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

Sorry I missed this comment. This should not be MustGetString, since my intention was to make specifying the endpoint via the command line optional. The reasoning is WithEndpoint() does not allow configuration of the URL path, so if that is needed, configuration via environment variable is required.

Copy link
Contributor

@christroger christroger Feb 2, 2022

Choose a reason for hiding this comment

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

In my Test, theMustGetString will just render an empty String as that is set as default.

@jzelinskie jzelinskie merged commit 1bd0f89 into jzelinskie:main Feb 1, 2022
@cjs cjs deleted the optional-endpoint branch Feb 9, 2022
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