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(exporter-logs-otlp-proto): programatic headers take precedence ov… #4351

Merged
merged 11 commits into from
Jan 3, 2024

Conversation

Vunovati
Copy link
Contributor

@Vunovati Vunovati commented Dec 8, 2023

…er environment variables

Which problem is this PR solving?

Exporters are not respecting the documented behaviour (link) when it comes to headers.

Settings configured programmatically take precedence over environment variables. Per-signal environment variables take precedence over non-per-signal environment variables.

Continuation of the previous PR #4334 based on the discussion
#4334 (review)

Fixes #2370

Short description of the changes

override OTEL_EXPORTER_OTLP_METRICS_HEADERS and OTEL_EXPORTER_OTLP_HEADERS with config.headers supplied to OTLPLogExporter and OTLPTraceExporter

Applied to:

  • @opentelemetry/exporter-logs-otlp-proto
  • @opentelemetry/exporter-logs-otlp-http
  • @opentelemetry/exporter-trace-otlp-proto
  • @opentelemetry/exporter-trace-otlp-http

Did not change anything in grpc exporters for traces and logs as setting headers for that protocol probably does not make sense:

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

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

Added additional tests testing that the programmatic settings take precedence.

  1. for headers
  2. for url, which was already working per spec but was not covered with unit tests

Checklist:

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

Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Merging #4351 (7bbf861) into main (0206181) will decrease coverage by 0.02%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 7bbf861 differs from pull request most recent head b02f746. Consider uploading reports for the commit b02f746 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4351      +/-   ##
==========================================
- Coverage   92.24%   92.22%   -0.02%     
==========================================
  Files         333      333              
  Lines        9459     9459              
  Branches     2009     2009              
==========================================
- Hits         8725     8724       -1     
- Misses        734      735       +1     
Files Coverage Δ
...ogs-otlp-http/src/platform/node/OTLPLogExporter.ts 100.00% <100.00%> (ø)
...gs-otlp-proto/src/platform/node/OTLPLogExporter.ts 100.00% <ø> (ø)
...e-otlp-http/src/platform/node/OTLPTraceExporter.ts 100.00% <ø> (ø)
...-otlp-proto/src/platform/node/OTLPTraceExporter.ts 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

@Vunovati Vunovati marked this pull request as ready for review December 12, 2023 13:56
@Vunovati Vunovati requested a review from a team December 12, 2023 13:56
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 good, thanks for taking care of this. 🙂

@Vunovati Vunovati requested a review from pichlermarc January 3, 2024 14:48
@pichlermarc pichlermarc merged commit ae0a3c5 into open-telemetry:main Jan 3, 2024
17 checks passed
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
open-telemetry#4351)

* fix(exporter-logs-otlp-proto): programatic headers take precedence over environment variables

* chore: update PR url in changelog

* chore: fix deletion of env var

* fix(exporter-logs-otlp-http): programatic headers take precedence over environment variables

* fix(exporter-trace-otlp-http): programatic headers take precedence over environment variables

* fix(exporter-trace-otlp-proto): programatic headers take precedence over environment variable

* chore: update CHANGELOG

---------

Co-authored-by: Marc Pichler <[email protected]>
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.

Code should take precedence over environment variables
2 participants