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.

Sign up for GitHub

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

Jump to bottom

Code should take precedence over environment variables #2370

Closed
1 task done
vreynolds opened this issue Jul 21, 2021 · 10 comments · Fixed by #4351
Closed
1 task done

Code should take precedence over environment variables #2370

vreynolds opened this issue Jul 21, 2021 · 10 comments · Fixed by #4351
Labels
never-stale up-for-grabs Good for taking. Extra help will be provided by maintainers

Comments

@vreynolds
Copy link
Contributor

The OTEL_EXPORTER_OTLP_HEADERS environment variable and its variants will clobber headers set in the code config. For example, see grpc setup or http setup.

The OTEL_EXPORTER_OTLP_ENDPOINT on the other hand, will prefer the code config, and fall back to the environment variable here.

They should probably at least behave consistently. Thoughts?

  • This only affects the JavaScript OpenTelemetry library
@vmarchaud
Copy link
Member

OTEL_EXPORTER_OTLP_HEADERS can be merged with code defined headers where as the OTEL_EXPORTER_OTLP_ENDPOINT can be merged with the code defined one, hence this behavior. Or maybe i misunderstood what you said ?

@vreynolds
Copy link
Contributor Author

The environment headers do get merged with the config ones, but the merge will also overwrite any duplicate header keys.

For example, if I define {foo: bar} in code config, and then set HEADERS:foo=baz, then the exporter will use baz (environment > code config). Whereas the endpoint has the opposite preference (code config > environment > default endpoint).

Does that make sense? I'm wondering if we should switch up the merge, so the environment headers get merged first, followed by code config headers. That way, if there is the same header specified in env and code, code will take precedence.

@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Dec 27, 2021
@dyladan dyladan removed the stale label Dec 27, 2021
@dyladan
Copy link
Member

dyladan commented Dec 27, 2021

In the case that headers are duplicated in code and env, the code should take precedence. We have had this come up in other places before too, just can't remember exactly where.

@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Feb 28, 2022
@dyladan
Copy link
Member

dyladan commented Mar 3, 2022

This is not stale

@dyladan dyladan removed the stale label Mar 3, 2022
@dyladan dyladan changed the title Headers environment variable vs code config precedence? Code should take precedence over environment variables Mar 3, 2022
@dyladan dyladan added the up-for-grabs Good for taking. Extra help will be provided by maintainers label Mar 3, 2022
@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label May 16, 2022
@github-actions
Copy link

github-actions bot commented Jun 6, 2022

This issue was closed because it has been stale for 14 days with no activity.

@github-actions github-actions bot closed this as completed Jun 6, 2022
@raphael-theriault-swi
Copy link
Contributor

This should probably be reopened

Vunovati added a commit to Vunovati/opentelemetry-js that referenced this issue Nov 30, 2023
@Vunovati
fix: programmatic url and headers take precedence in metric exporters (…
6ca5b05 
…open-telemetry#2370)
@Vunovati Vunovati mentioned this issue Nov 30, 2023
Merged
6 tasks
@pichlermarc pichlermarc reopened this Dec 6, 2023
pichlermarc pushed a commit that referenced this issue Dec 6, 2023
@Vunovati
fix: programmatic url and headers take precedence in metric exporters… (
4daa264 
#4334)

* fix: programmatic url and headers take precedence in metric exporters (#2370)

* chore: adjust grpc exporter metrics test

* chore(changelog): update changelog
@JamieDanielson
Copy link
Member

Looks to me like #4334 covered part of this, and we want a few more. Perhaps we can make this into a checklist to make it easier to see what still needs to get done?

I think this is the current list?

@Vunovati Vunovati mentioned this issue Dec 8, 2023
Merged
3 tasks
dyladan added a commit that referenced this issue Dec 13, 2023
@dyladan @pichlermarc
@trentm @lyzlisa @HyunnoH @seemk @Vunovati @renovate-bot
[next] merge changes from main (#4363)
ea8bfa2 
* Add Trent to approvers (#4311)

* chore(renovate): require dashboard approval for lerna updates (#4276)

* chore(ci): install semver globally to speed up "peer-api" workflow (#4270)

Closes: #4242

* fix(ci): remove token setup via environment variable from .npmrc (#4329)

* fix(instrumentation-http): resume responses when there is no response listener

Fixes a memory leak where unhandled response bodies pile up in node 20

* feat: add script to update changelogs on release preparation (#4315)

* feat: add script to update changelogs on releases

* fix: address comments

* Apply suggestions from code review

Co-authored-by: Trent Mick <[email protected]>

* fix: apply suggestions from code review

* fix: use packageJson.version instead of version

---------

Co-authored-by: Trent Mick <[email protected]>

* Fix event name

* test: make rawRequest HTTP-compliant

* Add node 20 to test matrix

* Enable old hash functions on 20

* Fix esm handling for iitm node 20

* Use err.code to make test more reliable

* Changelog

* nit: single import

* Remove unused files

* Add v20 to supported runtimes

* ci: add npm cache in actions/setup-node (#4271)

* feat(sdk-logs): add droppedAttributesCount field to ReadableLogRecord (#4289)

* feat(sdk-logs): add droppedAttributesCount field to ReadableLogRecord

* chore: check droppedAttributesCount value in test case

* feat(otlp-transformer): make toLogRecord() use ReadableLogRecord.droppedAttributesCount

---------

Co-authored-by: Marc Pichler <[email protected]>

* fix(api-logs): allow passing in TimeInput for LogRecord (#4345)

* fix: allow passing in TimeInput for LogRecord

* chore: update changelog

* fix: programmatic url and headers take precedence in metric exporters… (#4334)

* fix: programmatic url and headers take precedence in metric exporters (#2370)

* chore: adjust grpc exporter metrics test

* chore(changelog): update changelog

* fix(instrumentation-http): do not mutate given headers object for outgoing http requests (#4346)

Fixes: open-telemetry/opentelemetry-js-contrib#1609

* chore(deps): update actions/stale action to v9 (#4353)

* fix(deps): update dependency import-in-the-middle to v1.6.0 (#4357)

* chore(deps): update all patch versions (#4306)

* chore(ci): use node 20 in lint workflow (#4359)

* chore(deps): update dependency linkinator to v6 (#4237)

* fix(otlp-exporter-base): decrease default concurrency limit to 30 (#4211)

* fix(otlp-exporter-base): decrease concurrency limit to 30

* fix(changelog): add changelog entry

* chore(deps): use actions/checkout >4 instead of 4.0.0 exactly (#4361)

---------

Co-authored-by: Marc Pichler <[email protected]>
Co-authored-by: strivly <[email protected]>
Co-authored-by: Trent Mick <[email protected]>
Co-authored-by: lyzlisa <[email protected]>
Co-authored-by: Hyun Oh <[email protected]>
Co-authored-by: Siim Kallas <[email protected]>
Co-authored-by: Vladimir Adamić <[email protected]>
Co-authored-by: Mend Renovate <[email protected]>
rdeavila94 pushed a commit to rdeavila94/opentelemetry-js that referenced this issue Jan 3, 2024
@Vunovati @rdeavila94
fix: programmatic url and headers take precedence in metric exporters… (
591c776 
open-telemetry#4334)

* fix: programmatic url and headers take precedence in metric exporters (open-telemetry#2370)

* chore: adjust grpc exporter metrics test

* chore(changelog): update changelog
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this issue Sep 14, 2024
@Vunovati @Zirak
fix: programmatic url and headers take precedence in metric exporters… (
72c24b7 
open-telemetry#4334)

* fix: programmatic url and headers take precedence in metric exporters (open-telemetry#2370)

* chore: adjust grpc exporter metrics test

* chore(changelog): update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
never-stale up-for-grabs Good for taking. Extra help will be provided by maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(exporter-logs-otlp-proto): programatic headers take precedence ov… Vunovati/opentelemetry-js
6 participants
@dyladan @vmarchaud @vreynolds @pichlermarc @JamieDanielson @raphael-theriault-swi