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

feat: OTEL_EXPORTER_OTLP_ENDPOINT append version and signal #2438

Merged

Conversation

longility
Copy link
Contributor

@longility longility commented Aug 28, 2021

collector convention of appending the version and signal to the path (e.g. v1/traces or v1/metrics), if not present already

resolves #2436

…ppend version and signal

collector convention of appending the version and signal to the path
(e.g. v1/traces or v1/metrics), if not present already
@longility longility requested a review from a team August 28, 2021 15:42
@vmarchaud vmarchaud added the enhancement New feature or request label Aug 29, 2021
@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #2438 (c8074c0) into main (f0caa22) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2438   +/-   ##
=======================================
  Coverage   92.70%   92.71%           
=======================================
  Files         137      137           
  Lines        4993     4996    +3     
  Branches     1056     1057    +1     
=======================================
+ Hits         4629     4632    +3     
  Misses        364      364           
Impacted Files Coverage Δ
...kages/opentelemetry-exporter-collector/src/util.ts 100.00% <100.00%> (ø)

@dyladan
Copy link
Member

dyladan commented Aug 30, 2021

Can you please add an explanation to the proper use of the environment variables to the readme? This is likely very confusing to new users already and with this addition it becomes even more cryptic if you don't already know what is going on.

@longility
Copy link
Contributor Author

Can you please add an explanation to the proper use of the environment variables to the readme? This is likely very confusing to new users already and with this addition it becomes even more cryptic if you don't already know what is going on.

@dyladan Which section were you thinking of? Quick start, upgrade guidelines, or something else?

@longility
Copy link
Contributor Author

As I had more thought, I am basically just following the spec when I was doing this. Were you thinking of a guidance for env var in general that refers to the otel spec? If so, I think that should be a separate PR.

@dyladan
Copy link
Member

dyladan commented Aug 30, 2021

Can you please add an explanation to the proper use of the environment variables to the readme? This is likely very confusing to new users already and with this addition it becomes even more cryptic if you don't already know what is going on.

@dyladan Which section were you thinking of? Quick start, upgrade guidelines, or something else?

In the exporter readme, I would add a "configuration" section.

@longility
Copy link
Contributor Author

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

Requesting changes to use only one instance of appendResourcePathToUrlIfNotPresent function inside main exporter collector and then use it whenever you need it.

@longility longility requested a review from obecny August 30, 2021 19:11
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm, thx for changes

@dyladan dyladan changed the title fix: OTEL_EXPORTER_OTLP_ENDPOINT append version and signal feat: OTEL_EXPORTER_OTLP_ENDPOINT append version and signal Aug 30, 2021
@dyladan
Copy link
Member

dyladan commented Aug 30, 2021

Changed from fix to feat since this changes behavior

@dyladan dyladan merged commit dc574f7 into open-telemetry:main Aug 31, 2021
@dyladan dyladan mentioned this pull request Sep 13, 2021
@longility longility deleted the fix/exporter-collector-endpoint-path branch October 3, 2021 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code to support environment variables for exporter does not meet spec.
6 participants