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(otlp-http-exporter): update endpoint to match spec #2895

Merged
merged 30 commits into from
May 17, 2022

Conversation

svetlanabrennan
Copy link
Contributor

@svetlanabrennan svetlanabrennan commented Apr 11, 2022

Which problem is this PR solving?

Problem 1:
Spec says that OTEL_EXPORTER_OTLP_ENDPOINT is used for general endpoint so the http exporter doesn't have to check if a path is included in this endpoint. Our current implementation is checking for path in this endpoint.

Fixes #2816

Problem 2:
Spec says that per-signal variables (OTEL_EXPORTER_OTLP_<signal>_ENDPOINT) should add a root path / if the url contains no path part. Our current implementation doesn't check for root path and doesn't add root path.

Short description of the changes

  1. Renamed function from appendResourcePathToUrlIfNotPresent to appendResourcePathToUrl and removed logic around checking for path when user provides endpoint via OTEL_EXPORTER_OTLP_ENDPOINT.
  2. Added function called appendRootPathToUrlIfNeeded that will append root path / to url if needed.
  3. Updated tests around problem 1 above and added new tests for problem 2 above.
  4. Added utils unit test for testing appendResourcePathToUrl and appendRootPathToUrlIfNeeded.

Type of change

Please delete options that are not relevant.

  • 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

  • Unit Tests

Checklist:

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

Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
@codecov
Copy link

codecov bot commented Apr 25, 2022

Codecov Report

Merging #2895 (294e544) into main (a9c59da) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2895      +/-   ##
==========================================
+ Coverage   92.51%   92.55%   +0.03%     
==========================================
  Files         183      183              
  Lines        6029     6034       +5     
  Branches     1283     1284       +1     
==========================================
+ Hits         5578     5585       +7     
+ Misses        451      449       -2     
Impacted Files Coverage Δ
...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%> (ø)
...perimental/packages/otlp-exporter-base/src/util.ts 97.05% <100.00%> (+7.40%) ⬆️

svetlanabrennan and others added 11 commits April 26, 2022 10:22
Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
…spec

Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
…nal endpoints

Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
…path to url

Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
@svetlanabrennan svetlanabrennan marked this pull request as ready for review May 2, 2022 23:06
@svetlanabrennan svetlanabrennan requested a review from a team May 2, 2022 23:06
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! 🙂

@pichlermarc
Copy link
Member

/easycla

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OTLP/HTTP trace exporter does not use OTEL_EXPORTER_OTLP_ENDPOINT as a base URL correctly.
6 participants