-
Notifications
You must be signed in to change notification settings - Fork 1.2k
otlploggrpc: Add examples with self-signed certificates #6882
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6882 +/- ##
=====================================
Coverage 82.8% 82.8%
=====================================
Files 262 262
Lines 24339 24339
=====================================
+ Hits 20153 20154 +1
+ Misses 3811 3810 -1
Partials 375 375 🚀 New features to boost your workflow:
|
Hi @pellared , |
…s, use custom environment variable.
Co-authored-by: Robert Pająk <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>
This PR seems improperly synced with |
…ntion only to simplify the example
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.
I like this. I just added some cleanup comments. Hopefully these are my last comments 😉
Do you want to make similar PRs for other OTLP exporters (otlploghttp
, otlpmetricgrpc
, otlpmetrichttp
, otlptracegrpc
, otlptracehttp
)?
Co-authored-by: Robert Pająk <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>
Yes, I'd like to, if no other comments, will start to work on the other exporters |
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.
I made a little refactoring 63c83a4
@open-telemetry/go-approvers, PTAL
I have checked the otlploghttp module, it seems the example for self-signed certificate is not needed. It works fine when only specify the environment variable "OTEL_EXPORTER_OTLP_CERTIFICATE" for self-signed certificate TLS connection. The reason is otlploghttp.New(ctx) will load the TLS config from the environment variables (newConfig() in otlploghttp/config.go ) So otlploghttp.WithTLSClientConfig() is not needed, this is align with the README (https://pkg.go.dev/go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp)
|
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.
I have checked the otlploghttp module, it seems the example for self-signed certificate is not needed.
I quickly checked the otlploggrpc
code and docs and I would suspect that this should work the same way as for otlploghttp
. I have a strong feeling that my comment #6661 (comment) was incorrect (sorry for that).
Do you know of this issue is true for other gRPC OTLP exporters? I wouldn't be surprised if this is only a problem only in otlploggrpc
.
I have checked otlpmetricgrpc and otlptracegrpc, they both work fine, so you're right, it seems a problem only in otlploggrpc |
@xue20xi, given the latest comment I think we can close the PR. Do you want to work on fixing #6661 given it seems to be a bug and you already know what code should be invoked so that it works properly? I mean the examples here work and this is the code that should be invoke when e.g. |
Towards #6661