Traces server span and url fixes #80
Conversation
Codecov Report
@@ Coverage Diff @@
## main #80 +/- ##
==========================================
+ Coverage 45.71% 46.74% +1.02%
==========================================
Files 15 15
Lines 1166 1166
==========================================
+ Hits 533 545 +12
+ Misses 580 572 -8
+ Partials 53 49 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Good catch! Thanks for fixing. |
| @@ -180,7 +181,7 @@ func getMetricEndpointOptions(cfg *MetricsConfig) ([]otlpmetrichttp.Option, erro | |||
| if murl.Scheme == "http" || murl.Scheme == "unix" { | |||
| opts = append(opts, otlpmetrichttp.WithInsecure()) | |||
| } | |||
| if len(murl.Path) > 0 && murl.Path != "/" { | |||
| if len(murl.Path) > 0 && murl.Path != "/" && !strings.HasSuffix(murl.Path, "/v1/metrics") { | |||
There was a problem hiding this comment.
Hmmm I'm thinking in the case of someone willing to pass:
OTEL_EXPORTER_OTLP_METRICS_ENDPOINT=https://foo:8080/my/metrics, then the code will set the endpoint as https://foo:8080/my/metrics/v1/metrics. But it should remain untouched in that case, right?
I think then the code should be something like:
if cfg.MetricsEndpoint == "" && murl.Path != "/" {
opts = append(opts, otlpmetrichttp.WithURLPath(murl.Path+"/v1/metrics"))
}There was a problem hiding this comment.
Re-reviewing the OTEL spec: https://opentelemetry.io/docs/concepts/sdk-configuration/otlp-exporter-configuration/#otel_exporter_otlp_endpoint
It says that the OTEL_EXPORTER_OTLP_METRICS_ENDPOINT MUST end with /v1/traces so it is fine if, instead of returning an error, we silently add the path.
| @@ -186,8 +187,10 @@ func getTracesEndpointOptions(cfg *TracesConfig) ([]otlptracehttp.Option, error) | |||
| if murl.Scheme == "http" || murl.Scheme == "unix" { | |||
| opts = append(opts, otlptracehttp.WithInsecure()) | |||
| } | |||
| if len(murl.Path) > 0 && murl.Path != "/" { | |||
|
|
|||
| if len(murl.Path) > 0 && murl.Path != "/" && !strings.HasSuffix(murl.Path, "/v1/traces") { | |||
|
Thanks Mario! |
Traces server span and url fixes
This PR changes the type of the external span in traces to be SpanKind = SpanKindServer. When we have the external span as server, we are able to produce metrics with just trace spans. The Tempo span metric generator is able to produce metrics.
While testing this functionality I noticed a small issue when using the
OTEL_EXPORTER_OTLP_TRACES_ENDPOINTand theOTEL_EXPORTER_OTLP_METRICS_ENDPOINTenvironment variables. As per the OTel spec, if we are using the non-signal specific environment variable (i.e. not using OTEL_EXPORTER_OTLP_ENDPOINT) we need to pass in the full URL for the traces or the metrics, including the /v1/traces and /v1/metrics suffixes -> https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#example-2. However, if our code noticed anything in the URL path (other than/), we'd append /v1/traces|metrics, technically, appending twice.To fix this issue, I changed the code to not append /v1/traces|metrics if that's the suffix. I believe this will work in all cases.