-
Notifications
You must be signed in to change notification settings - Fork 805
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
feat: OTEL_EXPORTER_OTLP_ENDPOINT append version and signal #2438
Conversation
…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
Codecov Report
@@ 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
|
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? |
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. |
In the exporter readme, I would add a "configuration" section. |
@dyladan I added some documentation to this PR and you can see a preview of that here: https://github.com/open-telemetry/opentelemetry-js/blob/b23f4e8ccf8c92baa93be5e2322fc7f38d929a4d/packages/opentelemetry-exporter-collector/README.md#configuration-options-as-environment-variables |
There was a problem hiding this 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.
There was a problem hiding this 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
Changed from fix to feat since this changes behavior |
collector convention of appending the version and signal to the path (e.g. v1/traces or v1/metrics), if not present already
resolves #2436