-
Notifications
You must be signed in to change notification settings - Fork 795
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(exporter-collector): support config from env #2099 #2117
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2117 +/- ##
==========================================
+ Coverage 92.27% 92.69% +0.41%
==========================================
Files 122 141 +19
Lines 4066 5037 +971
Branches 833 1037 +204
==========================================
+ Hits 3752 4669 +917
- Misses 314 368 +54
|
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.
Generally looks good to me, thx for changes.
One last thing.
For all different exporter variations the example examples/collector-exporter-node/
is being used.
There is a docker container which can be run and then all collector variations can be tested to see if data is being passed correctly. After changing the default port I can assume it will not be working. Could you please update the docker to ensure it is still working fine (use the latest docker image 0.25
)? And as last thing the readme.md in line 8 we have information that the exporter has been tested with a certain version of collector.
Would be nice to update it too then.
Sure, but i think it would make sense to make test with the collector (end to end pretty much) itself in our pipeline so this doesn't require manual testing in the future |
that would be best, but it would require probably much more effort, but definitely nice to have |
I switched back the http port because its really unclear how its supposed to be:
A discussion is still ongoing on the collector side too: open-telemetry/opentelemetry-collector#1256 |
2ea34db
to
91d3e0f
Compare
@obecny Rebased + tested with latest collector version (0.25.0) and its works fine ! |
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
91d3e0f
to
b3a2647
Compare
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
b3a2647
to
6dd634d
Compare
Friendly ping here too @dyladan :) |
this is a new PR addressing feedbacks made on #2101. Some changes not directly related to the main issue that i've made:
4317
now: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/otlp.md#otlphttp-default-portFixes #2099