opentelemetry: Overwrite defaults with standard OTLP exporter env vars#5350
opentelemetry: Overwrite defaults with standard OTLP exporter env vars#5350
Conversation
df5a0e8 to
c0ec91b
Compare
AgnesToulet
left a comment
There was a problem hiding this comment.
Looks good except for OTEL_EXPORTER_OTLP_[METRICS_]INSECURE and OTEL_EXPORTER_OTLP_[METRICS_]ENDPOINT for HTTP configurations that will behave differently from what's described in the OpenTelemetry doc.
| } | ||
|
|
||
| if err := cfg.Validate(); err != nil { | ||
| // TODO: check why k6's still exiting with 255 |
There was a problem hiding this comment.
Yes, it seemed to me that this comment was outdated, that's why I removed it.
I checked it by running k6 with an invalid configuration (see below), and checking that the exit status is 104, which corresponds to the invalid configuration exit code.
❯ K6_OTEL_EXPORTER_PROTOCOL=error go run main.go run -o opentelemetry examples/docker-compose/opentelemetry/script.js
/\ Grafana /‾‾/
/\ / \ |\ __ / /
/ \/ \ | |/ / / ‾‾\
/ \ | ( | (‾) |
/ __________ \ |_|\_\ \_____/
Init [--------------------------------------]
default [--------------------------------------]
ERRO[0000] could not create the 'opentelemetry' output: error validating OpenTelemetry output config: unsupported exporter protocol "error", only "grpc" and "http/protobuf" are supported
exit status 104| if exporterInsecureBoolVar != "" { | ||
| exporterInsecureBool, err := strconv.ParseBool(exporterInsecureBoolVar) | ||
| if err != nil { | ||
| return Config{}, err | ||
| } | ||
|
|
||
| stdCfg.GRPCExporterInsecure = null.BoolFrom(exporterInsecureBool) | ||
| stdCfg.HTTPExporterInsecure = null.BoolFrom(exporterInsecureBool) | ||
| } | ||
|
|
||
| if exporterEndpoint, ok := env["OTEL_EXPORTER_OTLP_METRICS_ENDPOINT"]; ok { | ||
| stdCfg.GRPCExporterEndpoint = null.StringFrom(exporterEndpoint) | ||
| stdCfg.HTTPExporterEndpoint = null.StringFrom(exporterEndpoint) | ||
| } else if exporterEndpoint, ok := env["OTEL_EXPORTER_OTLP_ENDPOINT"]; ok { | ||
| stdCfg.GRPCExporterEndpoint = null.StringFrom(exporterEndpoint) | ||
| stdCfg.HTTPExporterEndpoint = null.StringFrom(exporterEndpoint) | ||
| } |
There was a problem hiding this comment.
I think we have an issue to support these options for the HTTP exporter. I haven't tested it but from k6 doc:
K6_OTEL_HTTP_EXPORTER_ENDPOINTConfigures the HTTP exporter endpoint. Must be host and port only, without scheme. Default is localhost:4318.
And from OpenTelemetry doc:
Additionally, the option MUST accept a URL with a scheme of either http or https. A scheme of https indicates a secure connection and takes precedence over the insecure configuration setting. A scheme of http indicates an insecure connection and takes precedence over the insecure configuration setting.
So first, users using the endpoint env variable like they are used will have errors when using http and https schemes. And if we fix that, we'll need to keep in mind that this should override the insecure setting.
There was a problem hiding this comment.
Good catch @AgnesToulet! Thanks! 🙌🏻 🙇🏻
How does 8633d53 look like? 🤔 I tried to apply what's stated in the aforementioned docs.
| config, err := GetConsolidatedConfig(testCase.jsonRaw, testCase.env) | ||
|
|
||
| logger := logrus.New() | ||
| logger.SetOutput(io.Discard) |
There was a problem hiding this comment.
Could we test warnIfConfigMismatch here instead of discarding the logs?
There was a problem hiding this comment.
Good idea! I have added a new test in 4d3a1c2. I preferred this, because TestConfig is way more generic, so we avoid having such specific assertion within a very generic test function.
efda2e9 to
4d3a1c2
Compare
AgnesToulet
left a comment
There was a problem hiding this comment.
Looks pretty good to me, I still have two questions around consistency with the OTEL spec.
| // HTTP requires a port, gRPC doesn't. | ||
| if isHTTP && port == "" { | ||
| return null.String{}, null.Bool{}, errors.New("endpoint must contain host and port") | ||
| } |
There was a problem hiding this comment.
Where did you read that HTTP requires a port? This is not what I understand from this part (in this paragraph):
An SDK MUST NOT modify the URL in ways other than specified above. That also means, if the port is empty or not given, TCP port 80 is the default for the http scheme and TCP port 443 is the default for the https scheme, as per the usual rules for these schemes (RFC 7230).
There was a problem hiding this comment.
This is historical behavior from the k6 extension itself, rather than something specific to OTLP specification. See:
Configures the HTTP exporter endpoint. Must be host and port only, without scheme. Default is localhost:4318.
(https://grafana.com/docs/k6/latest/results-output/real-time/opentelemetry/)
But your comment made me wonder, should I perhaps append the default port, in case we get the endpoint from standard OTLP variables and it doesn't include already a port? 🤔
| exporterEndpointString = exporterEndpointVal | ||
| } else if exporterEndpointVal, ok := env["OTEL_EXPORTER_OTLP_ENDPOINT"]; ok { | ||
| exporterEndpointVar = "OTEL_EXPORTER_OTLP_ENDPOINT" | ||
| exporterEndpointString = exporterEndpointVal |
There was a problem hiding this comment.
I wonder from this paragraph if we should add /v1/metrics to this value.
There was a problem hiding this comment.
Makes sense, I'll try to make sure this gets appended if we populate the endpoint from "standard" OTLP variables 👍🏻
What?
Takes into consideration OTLP exporter environment variables (documented here, as overwrites for the configuration defaults, as discussed in #4571.
It also writes a warning log line in case there are discrepancies (gRPC vs HTTP) within the same configuration block (env vars, JSON, etc), as suggested here and here.
Why?
Because it makes sense to fallback to those "standard" environment variables, where there's no one specific to k6 defined. As, environments where OTLP is broadly use may already be relying on those, and this would much the overall experience smoother and friendlier.
The warning is also another improvement in terms of user experience.
Checklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
Closes #4571
Closes #4070 (I guess)