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.

Sign up for GitHub

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

Jump to bottom

make OTel endpoint optional and use defaults if note set #4

Merged
merged 5 commits into from Feb 1, 2022
Merged

make OTel endpoint optional and use defaults if note set #4

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
563630c
make endpoint optional and use defaults if note set
cjs Jan 27, 2022
b759719
add flag to customize tracer, with fallback to w3c
christroger Jan 31, 2022
c4a5fd9
add comment about headers to flag, update appending endpoints
christroger Jan 31, 2022
e4781fe
expand on endpoint comment
christroger Jan 31, 2022
22acbd3
update function to get enpoint flag
christroger Feb 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 25 additions & 5 deletions cobrautil.go
View file
Open in desktop
Expand Up @@ -155,7 +155,7 @@ func RegisterOpenTelemetryFlags(flags *pflag.FlagSet, flagPrefix, serviceName st
prefixed := prefixJoiner(stringz.DefaultEmpty(flagPrefix, "otel"))

flags.String(prefixed("provider"), "none", `OpenTelemetry provider for tracing ("none", "jaeger, otlphttp", "otlpgrpc")`)
flags.String(prefixed("endpoint"), "http://collector:14268/api/traces", "collector endpoint")
flags.String(prefixed("endpoint"), "", "OpenTelemetry collector endpoint")
flags.String(prefixed("service-name"), serviceName, "service name for trace data")
}

Expand All @@ -173,24 +173,44 @@ 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

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

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

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

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.


var exporter trace.SpanExporter
var err error

// If endpoint is not set, the clients are configured via the OpenTelemetry environment variables or
// default values.
jzelinskie marked this conversation as resolved.
Show resolved Hide resolved
// See: https://github.com/open-telemetry/opentelemetry-go/tree/main/exporters/otlp/otlptrace#environment-variables
// or https://github.com/open-telemetry/opentelemetry-go/tree/main/exporters/jaeger#environment-variables
switch provider {
case "none":
// Nothing.
case "jaeger":
exporter, err := jaeger.New(jaeger.WithCollectorEndpoint(jaeger.WithEndpoint(MustGetString(cmd, prefixed("endpoint")))))
if endpoint != "" {
exporter, err = jaeger.New(jaeger.WithCollectorEndpoint(jaeger.WithEndpoint(endpoint)))
} else {
exporter, err = jaeger.New(jaeger.WithCollectorEndpoint())
}
Copy link
Owner

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

Choose a reason for hiding this comment

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

addressed in c4a5fd9

if err != nil {
return err
}
return initOtelTracer(exporter, serviceName)
case "otlphttp":
exporter, err := otlptrace.New(context.Background(), otlptracehttp.NewClient())
if endpoint != "" {
exporter, err = otlptrace.New(context.Background(), otlptracehttp.NewClient(otlptracehttp.WithEndpoint(endpoint)))
} else {
exporter, err = otlptrace.New(context.Background(), otlptracehttp.NewClient())
}
if err != nil {
return err
}
return initOtelTracer(exporter, serviceName)
case "otlpgrpc":
exporter, err := otlptrace.New(context.Background(), otlptracegrpc.NewClient())
if endpoint != "" {
exporter, err = otlptrace.New(context.Background(), otlptracegrpc.NewClient(otlptracegrpc.WithEndpoint(endpoint)))
} else {
exporter, err = otlptrace.New(context.Background(), otlptracegrpc.NewClient())
}
if err != nil {
return err
}
Expand Down