Fix otlp exporters default endpoints #4155
Conversation
remychantenay
requested review from
MrAlias,
Aneurysm9,
evantorrie,
XSAM,
dashpole,
MadVikingGod,
pellared,
hanyuancheung and
dmathieu
as code owners
| @@ -137,10 +137,9 @@ func TestConfig(t *testing.T) { | |||
| assert.Len(t, rCh, 0, "failed HTTP responses did not occur") | |||
| }) | |||
|
|
|||
| t.Run("WithURLPath", func(t *testing.T) { | |||
There was a problem hiding this comment.
Note to reviewer(s) – This test is a dupe, see L151 below.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4155 +/- ##
=======================================
- Coverage 83.3% 83.3% -0.1%
=======================================
Files 181 181
Lines 13928 13955 +27
=======================================
+ Hits 11615 11634 +19
- Misses 2092 2098 +6
- Partials 221 223 +2
|
remychantenay
force-pushed
the
fix-otlp-exporters-default-endpoints
branch
from
f3418d6
to
8045992
Compare
| @@ -89,16 +89,23 @@ func NewHTTPConfig(opts ...HTTPOption) Config { | |||
| URLPath: DefaultMetricsPath, | |||
| Compression: NoCompression, | |||
| Timeout: DefaultTimeout, | |||
| Insecure: true, | |||
There was a problem hiding this comment.
The default for this should be false according to the specification: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/protocol/exporter.md#configuration-options
Why is this set to true here?
There was a problem hiding this comment.
I just want to add a not that the specification is not worded perfectly.
This option only applies to OTLP/gRPC when an endpoint is provided without the http or https scheme.
insecure is by default true when an endpoint is provided without the http or https scheme.
Still, I think that we should keep the false as default and not interpret the Insecure field if the HTTP scheme is defined. (I guess this is what @MrAlias has in mind)
There was a problem hiding this comment.
Thank you both. Looks like there is a bit of confusion from my end. I will re-read the specs with rested eyes over the weekend.
There was a problem hiding this comment.
This option only applies to OTLP/gRPC when an endpoint is provided without the http or https scheme - OTLP/HTTP always uses the scheme provided for the endpoint.
My interpretation of this is that OTLP/HTTP endpoints must include a scheme (as noted n the specification). But the scheme overrides whatever setting there may be for Insecure in the config (i.e. it's a noop setting for OTLP/HTTP).
For example, if the OTLP/HTTP endpoint starts with http, then the exporter should act as though Insecure= true, but if the scheme in the HTTP endpoint is https, it should act as ifInsecure=false. The wording seems to indicate that the user setting for Insecure shouldn't be changed, but rather the exporter should just use the appropriate setting regardless of the Insecure value in the config.
For OTLP/gRPC, there are more options for specifying the endpoint. But if the gRPC endpoint includes an http or https scheme, then the same "ignore what the setting says and act as though it is whatever the scheme says" applies the same as in OTLP/HTTP.
But if the gRPC endpoint does not include an http or https scheme, then the Insecure setting should be taken into account and should default to Insecure=false.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
This is related to #4016 |
Co-authored-by: Damien Mathieu <[email protected]>
|
I do not think it is possible to fix the issue without making a breaking change or creating new API. What do you think? I plan to close this PR by the end of the week if I do not get any feedback. |
Hey @pellared, My sincere apologies for the radio silence. I was abroad, got caught up in other stuff, and never really came back to this. I don't want to be a burden more than I already have been. Also, I will be offline from tomorrow until the end of the month, and considering the time that has gone by since I opened this PR, I think it's best to close this PR. Happy to do so. Again, my sincere apologies for the inconvenience to all of you. |
|
@remychantenay There is nothing you need to be sorry about. I would rather blame myself for even rising the issue 🙈 Thank your for contribution 👍 |
Fixes #4147