-
Notifications
You must be signed in to change notification settings - Fork 582
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
config: add support for certificate configuration #6376
config: add support for certificate configuration #6376
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6376 +/- ##
=====================================
Coverage 67.8% 67.9%
=====================================
Files 200 200
Lines 16641 16689 +48
=====================================
+ Hits 11297 11342 +45
- Misses 4999 5001 +2
- Partials 345 346 +1
|
Not sure if the test failure is related to my change, i don't think so but i could be wrong:
|
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'm not sure if you consider HTTP (as opposed to GRPC) config to be in scope for this PR, but the otlpHTTPLogExporter
has a similar problem - http in this context can also mean https
config/log.go
Outdated
@@ -178,6 +180,13 @@ func otlpGRPCLogExporter(ctx context.Context, otlpConfig *OTLP) (sdklog.Exporter | |||
if u.Scheme == "http" { | |||
opts = append(opts, otlploggrpc.WithInsecure()) | |||
} | |||
if otlpConfig.Certificate != nil { |
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'm curious about why this was put here and not the outer scope on line 190/191 as a sibling of if otlp.Config.Compression != nil {
? the other if statements in the inner block pertain to the result of url.ParseRequestURI
, so if you think it's better to put otlpConfig checks in this block, it would be good to understand when to add them here and when to add them in the outer scope
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 ended up moving the code after thinking about this further. originally i thought it made sense to only have it in the case where an endpoint was configured, but after rethinking through it i moved it out at the same level as compression and the other parameters. figured if there was a problem with the cert config it would make sense to surface it regardless of what the endpoint configuration is
I was planning on submitting a separate PR for the http exporter |
I'll do the rest of the changes if you're okay with that. Some of the other fields require overwriting this change anyway |
I created #6378 which is based on this PR |
Ah I missed your comment, i ended up implementing the http certificate field in this same PR |
28cb287
to
d57fc1a
Compare
d172dfc
to
8d7664e
Compare
Fixes open-telemetry#6351 Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
8d7664e
to
2cdf0ca
Compare
Signed-off-by: Alex Boten <[email protected]>
We need a second approval to merge. |
Part of #6351