opentelemetry-collector
opentelemetry-collector copied to clipboard
[exporter/otlp] Fix unix socket blocked with valid
Description
Fixes OTLP exporter config validation for Unix socket endpoints. Splits validation for host/port and Unix socket, keeping backward compatibility.
Link to tracking issue
Fixes #13642
Testing
• Added tests for DNS, HTTP, HTTPS, Unix socket, and other schemes. • Updated sanitize endpoint test; behavior unchanged. • Unix socket test covered.
Documentation
@tomatopunk Thanks for the thorough PR. I think we should see if we can solve this in configgrpc, ideally using the grpc-go library. Do you think if we remove the failing validation from the OTLP exporter, we could instead validate the connection URI using an underlying library like in https://github.com/open-telemetry/opentelemetry-collector/pull/13630?
@tomatopunk Thanks for the thorough PR. I think we should see if we can solve this in configgrpc, ideally using the grpc-go library. Do you think if we remove the failing validation from the OTLP exporter, we could instead validate the connection URI using an underlying library like in #13630?
I agree. I have removed the original validation code and now use configgrpc.Valid() for validation. Additionally, I deleted some of the previous test cases, since we can rely on configgrpc to guarantee their correctness.
Hey @evan-bradley, Just checking in — is there anything blocking this PR right now?
Do I need to make any more changes, or are we just waiting on #13630 to be merged before it can go through?
It doesn’t bring any breaking changes — it only removes the host port valid for the endpoint. Just trying to get any info on what might be holding it up :)
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 91.58%. Comparing base (ed2d996) to head (2d810e3).
:warning: Report is 283 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #13671 +/- ##
==========================================
+ Coverage 87.63% 91.58% +3.94%
==========================================
Files 632 633 +1
Lines 39697 39478 -219
==========================================
+ Hits 34790 36154 +1364
+ Misses 3660 2564 -1096
+ Partials 1247 760 -487
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
This PR was marked stale due to lack of activity. It will be closed in 14 days.
Hey @evan-bradley and team, I noticed that #13630 was closed due to inactivity. Are you still working on this issue, or is it okay for me to continue pushing it forward?
My plan is to remove the failing validation from the OTLP exporter and implement some static authentication in the underlying configgrpc library.
Once these changes are completed, is there a chance they will be approved?
This PR was marked stale due to lack of activity. It will be closed in 14 days.
This PR was marked stale due to lack of activity. It will be closed in 14 days.
This PR was marked stale due to lack of activity. It will be closed in 14 days.
Is there anything I can do to help move this along?
Is there anything I can do to help move this along?
This is a great question. If you want to see this issue resolved, I think the only way is for someone with merge privileges to address it themselves.
I’m not sure what else I can do to help get it merged, so I will go ahead and close this issue.