[otlpexporter] support validation for dns:https:// and dns:https:/// #10450
Conversation
This allows users to continue using the recommended dns:https://authority/host:port notation when needing to specify a custom authority. Fixes open-telemetry#10449 Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10450 +/- ##
==========================================
- Coverage 92.35% 92.34% -0.01%
==========================================
Files 386 393 +7
Lines 18408 18621 +213
==========================================
+ Hits 17001 17196 +195
- Misses 1053 1068 +15
- Partials 354 357 +3 ☔ View full report in Codecov by Sentry. |
exporter/otlpexporter/config_test.go
Outdated
| @@ -134,6 +134,17 @@ func TestUnmarshalInvalidConfig(t *testing.T) { | |||
| func TestValidDNSEndpoint(t *testing.T) { | |||
| factory := NewFactory() | |||
| cfg := factory.CreateDefaultConfig().(*Config) | |||
| cfg.Endpoint = "dns:https:///backend.example.com:4317" | |||
| cfg.Endpoint = "dns:https://backend.example.com:4317" | |||
There was a problem hiding this comment.
Is this a valid dns endpoint? I though it would need either /// in a row or another / somewhere to separate the authority and the path
There was a problem hiding this comment.
it's a good point, according to the grpc docs, the following is valid:
dns:[//authority/]host[:port] -- DNS (default)
I don't know how much more validation we should be introducing here
There was a problem hiding this comment.
The new string looks invalid to me, I'm surprised an error is not returned. I'm interpreting the regex as "if // is present, there must be a third /". What am I missing
There was a problem hiding this comment.
let me add a test for this case. i agree that it should return an error
There was a problem hiding this comment.
Oh I see how this test works now. Currently the validation only checks that there is a port and that it is a valid integer, nothing else. We should add full dns endpoint validation, but since the current functionality breaks users ability to use authorities in their dns endpoint I think we should add that later and merge this now.
For the sake of having a proper endpoint:
| cfg.Endpoint = "dns:https://backend.example.com:4317" | |
| cfg.Endpoint = "dns:https://authority/backend.example.com:4317" |
Signed-off-by: Alex Boten <[email protected]>
codeboten
changed the title
Signed-off-by: Alex Boten <[email protected]>
TylerHelmuth
added
the
ready-to-merge
This allows users to continue using the recommended dns:https://authority/host:port notation when needing to specify a custom authority.
Fixes #10449