[otlp exporter] Validate exporter endpoint has port #9632
Conversation
|
|
atmask
force-pushed
the
issue9505-otlp-port
branch
from
f66ddcc to
80c961f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9632 +/- ##
=======================================
Coverage 91.13% 91.13%
=======================================
Files 353 353
Lines 18728 18736 +8
=======================================
+ Hits 17067 17075 +8
Misses 1333 1333
Partials 328 328 ☔ View full report in Codecov by Sentry. |
exporter/otlpexporter/config.go
Outdated
| @@ -25,7 +27,26 @@ func (c *Config) Validate() error { | |||
| if c.SanitizedEndpoint() == "" { | |||
| return errors.New(`requires a non-empty "endpoint"`) | |||
| } | |||
|
|
|||
| // Validate endpoint has a valid port | |||
| port, err := c.endpointPort() | |||
There was a problem hiding this comment.
We have a default port, why not using that if not provided?
There was a problem hiding this comment.
Issue 9505 suggested either validation or using a default port. The config struct for the otlpexporter only takes an endpoint string which get's passed through into the configgrp config struct. I'm not sure where a good place to set up the default port would be in the otlp exporter. I don't think that the validation function is an appropriate place to do this and there is no constructor for this config, just the factory's createDefaultConfig function.
atmask
force-pushed
the
issue9505-otlp-port
branch
from
2495607 to
e9f8173
Compare
|
Issue raised in otel-collector-contrib: open-telemetry/opentelemetry-collector-contrib#31426 Related PR from otel-collector-contrib to update loadbalancingexporter for the change in otlp config validation: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/31425/files |
atmask
force-pushed
the
issue9505-otlp-port
branch
3 times, most recently
from
f3c3672 to
69b70e1
Compare
… unit test (#31425) **Description:** As described in the otel-collector [issue 9505](open-telemetry/opentelemetry-collector#9505), the otlpexporter does not function correctly if no port is defined. To resolve this, the otlp config validation has been updated to fail fast when the endpoint within an otlp config does not have a port specified. The loadbalancingexporter config has the otlp exporter config as a dependency, however, the loadbalancing exporter does not define a port on the otlpexporter config in two places: - default config from factory - testdata contents This is currently a blocker to the contrib tests for the [PR](open-telemetry/opentelemetry-collector#9632) to resolve issue 9505 Relates to: open-telemetry/opentelemetry-collector#9523 #31371 #31381 **Link to tracking Issue:** otel-collector-contrib: [issue 31426](#31426) Arises from otel-collector [issue 9505](open-telemetry/opentelemetry-collector#9505) **Testing:** Used `replace` to test loadbalancingexporter changes pass tests successfully when using the otlpexporter changes from [PR](open-telemetry/opentelemetry-collector#9632)
atmask
force-pushed
the
issue9505-otlp-port
branch
2 times, most recently
from
966ade0 to
7b871bf
Compare
atmask
force-pushed
the
issue9505-otlp-port
branch
2 times, most recently
from
8372f90 to
08c3b64
Compare
|
@djaglowski can you please give this a review. Should be ready to go in. The tests are passing locally and the blocking otel-contrib issue has been resolved by open-telemetry/opentelemetry-collector-contrib#31425. If the PR stays open I have to keep updating it as main is getting updated |
atmask
force-pushed
the
issue9505-otlp-port
branch
from
08c3b64 to
e9ccb0f
Compare
… unit test (open-telemetry#31425) **Description:** As described in the otel-collector [issue 9505](open-telemetry/opentelemetry-collector#9505), the otlpexporter does not function correctly if no port is defined. To resolve this, the otlp config validation has been updated to fail fast when the endpoint within an otlp config does not have a port specified. The loadbalancingexporter config has the otlp exporter config as a dependency, however, the loadbalancing exporter does not define a port on the otlpexporter config in two places: - default config from factory - testdata contents This is currently a blocker to the contrib tests for the [PR](open-telemetry/opentelemetry-collector#9632) to resolve issue 9505 Relates to: open-telemetry/opentelemetry-collector#9523 open-telemetry#31371 open-telemetry#31381 **Link to tracking Issue:** otel-collector-contrib: [issue 31426](open-telemetry#31426) Arises from otel-collector [issue 9505](open-telemetry/opentelemetry-collector#9505) **Testing:** Used `replace` to test loadbalancingexporter changes pass tests successfully when using the otlpexporter changes from [PR](open-telemetry/opentelemetry-collector#9632)
atmask
force-pushed
the
issue9505-otlp-port
branch
from
e9ccb0f to
7a5ff01
Compare
… defeined in the endpoint. Add additional unit test data for this case.
atmask
force-pushed
the
issue9505-otlp-port
branch
from
7a5ff01 to
3c5a421
Compare
|
Just updated to fix the lint issue for new commits going into main and rebased on main. @TylerHelmuth could you please review this or poke a codeowner to push this PR through so that it can get closed off? |
|
I think using the default port is better, but we can add the validation for now since it's not working without specifying the port explicitly anyway. I think we don't have clear guidance on how components should mutate config structs passed to them when it's required, like in this case. It'd be great to have an issue for that so we can track it separately. |
Description: This PR updates the otlp exporter config validation to ensure that the "endpoint" specified for the exporter includes a port. The goal of this is to fail fast if the configuration is invalid instead of waiting for an error to arise. The PR adds a function to the ClientConfig defined in configgrpc that parses the port defined in the endpoint. The otlp exporter uses this port parsing to validate that
Link to tracking Issue: Issue 9505
Testing:
configgrpc:
otlpexporter: