fix(exporter/otlp): align endpoint environment varaible handling with spec #1506
Conversation
|
Looks like the failing test is the same one as the one that's failing on |
pichlermarc
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam and
robbkidd
as code owners
There was a problem hiding this comment.
LGTM. I have minor concerns that some people could be relying on the broken behavior, but this definitely a bug that should be fixed. Thanks @pichlermarc!
Fully agree, thank you for the review 🙂 |
| # append '/' if not present | ||
| non_signal_specific_endpoint += '/' unless non_signal_specific_endpoint.end_with?('/') |
There was a problem hiding this comment.
This is not necessary. The URI.join call below will insert the / if required.
There was a problem hiding this comment.
Hmm, I could be wrong (this is the first time I'm using ruby). I also initially assumed it does, but trying it out in irb:
$ irb
3.2.0 :001 > require('uri')
=> true
3.2.0 :002 > URI.join('http://test.com/api','v2/url')
=> #<URI::HTTP http://test.com/v2/url>
drops api from the path. I think it may be debatable if it is directly intended by the spec here to append it. However, I think many language implementations do append '/' if it's not there (examples: JavaScript, Python, Java)
| begin | ||
| URI.join(non_signal_specific_endpoint, 'v1/traces') | ||
| rescue URI::InvalidURIError | ||
| raise ArgumentError, "invalid url for OTLP::Exporter #{non_signal_specific_endpoint}" |
There was a problem hiding this comment.
I strongly prefer to raise ArgumentError from the public method rather than a private one. It is more informative for the caller. Since this case is so specific, I think it would also be useful to call out the environment variable, in case the user wonders where the value came from.
There was a problem hiding this comment.
I'm re-raising in the constructor now to hide the private method f2748b7, couldn't pull everything in the constructor unfortunately as complexity ended up being too high and rubocop failed.
| certificate_file: OpenTelemetry::Common::Utilities.config_opt('OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE', 'OTEL_EXPORTER_OTLP_CERTIFICATE'), | ||
| ssl_verify_mode: Exporter.ssl_verify_mode, | ||
| headers: OpenTelemetry::Common::Utilities.config_opt('OTEL_EXPORTER_OTLP_TRACES_HEADERS', 'OTEL_EXPORTER_OTLP_HEADERS', default: {}), | ||
| compression: OpenTelemetry::Common::Utilities.config_opt('OTEL_EXPORTER_OTLP_TRACES_COMPRESSION', 'OTEL_EXPORTER_OTLP_COMPRESSION', default: 'gzip'), | ||
| timeout: OpenTelemetry::Common::Utilities.config_opt('OTEL_EXPORTER_OTLP_TRACES_TIMEOUT', 'OTEL_EXPORTER_OTLP_TIMEOUT', default: 10), | ||
| metrics_reporter: nil) | ||
| raise ArgumentError, "invalid url for OTLP::Exporter #{endpoint}" unless OpenTelemetry::Common::Utilities.valid_url?(endpoint) | ||
| @uri = determine_url(endpoint) |
There was a problem hiding this comment.
| @uri = determine_url(endpoint) | |
| endpoint ||= ENV['OTEL_EXPORTER_OTLP_TRACES_ENDPOINT'] | |
| @uri = begin | |
| if endpoint.nil? | |
| endpoint = ENV['OTEL_EXPORTER_OTLP_ENDPOINT'] || 'http://localhost:4318' | |
| URI.join(endpoint, 'v1/traces') | |
| elsif endpoint.strip.empty? | |
| raise ArgumentError, "invalid url for OTLP::Exporter #{endpoint}" | |
| else | |
| URI(endpoint) | |
| end | |
| rescue URI::Error | |
| raise ArgumentError, "invalid url for OTLP::Exporter #{endpoint}" | |
| end |
I think this captures the essence of the rules while raising as close to the public interface as possible.
There was a problem hiding this comment.
Yes, that's better (using some language features that I didn't know yet, nice 😄). Thanks for the suggestion 🙌
Accepted the suggestion in 40204f9, but included appending / (see #1506 (comment))
There was a problem hiding this comment.
Had to make some changes to make this pass the rubocop rules as complexity was too high f2748b7
Co-authored-by: Ariel Valentin <[email protected]>
|
@arielvalentin @fbogsany can you confirm that your suggestions have been properly addressed? |
This PR fixes the non spec compliant behavior outlined in #1505 🙂
Fixes #1505