Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(exporters): do not append trailing '/' when URL contains path #3274

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Sep 23, 2022

Which problem is this PR solving?

When a signal specific environment variable (OTEL_EXPORTER_OTLP_<signal>_ENDPOINT) was passed with a url that contains a path not ending in v1/traces or v1/metrics, a / was unexpectedly appended.

Fixes #3269

Edit for clarification:

From the spec:

For the per-signal variables (OTEL_EXPORTER_OTLP_<signal>_ENDPOINT), the URL MUST be used as-is without any modification. The only exception is that if an URL contains no path part, the root path / MUST be used (see Example 2).

there might be a path part that does not end in /v1/traces or /v1/metrics. With the previous implementation, it would append / to that path, which can result in 404s, see #3269

Short description of the changes

Now parsing the URL on initialization and adding the / only when the path is empty.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Existing unit tests
  • Added additional unit tests

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@pichlermarc pichlermarc marked this pull request as ready for review September 23, 2022 09:33
@pichlermarc pichlermarc requested a review from a team September 23, 2022 09:33
@codecov
Copy link

codecov bot commented Sep 23, 2022

Codecov Report

Merging #3274 (5c92d23) into main (e91cac5) will decrease coverage by 0.01%.
The diff coverage is 84.61%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3274      +/-   ##
==========================================
- Coverage   93.44%   93.43%   -0.02%     
==========================================
  Files         241      241              
  Lines        7249     7253       +4     
  Branches     1507     1507              
==========================================
+ Hits         6774     6777       +3     
- Misses        475      476       +1     
Impacted Files Coverage Δ
...perimental/packages/otlp-exporter-base/src/util.ts 94.73% <77.77%> (-2.33%) ⬇️
...e-otlp-http/src/platform/node/OTLPTraceExporter.ts 100.00% <100.00%> (ø)
...exporter-trace-otlp-proto/src/OTLPTraceExporter.ts 100.00% <100.00%> (ø)
...-otlp-http/src/platform/node/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...orter-metrics-otlp-proto/src/OTLPMetricExporter.ts 100.00% <100.00%> (ø)

@svetlanabrennan
Copy link
Contributor

Good catch. Thanks for fixing it! 😀

@pichlermarc pichlermarc added the bug Something isn't working label Sep 30, 2022
@pichlermarc pichlermarc added the priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect label Sep 30, 2022
@dyladan dyladan merged commit 1864a9a into open-telemetry:main Oct 5, 2022
@dyladan dyladan deleted the fix/unexpected-root-path-added branch October 5, 2022 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unexpected trailing / appended to endpoint configured via OTEL_EXPORTER_OTLP_TRACES_ENDPOINT
4 participants