Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(exporter/otlp): align endpoint environment varaible handling with spec #1506

Merged
merged 13 commits into from
Oct 7, 2023
Merged

fix(exporter/otlp): align endpoint environment varaible handling with spec #1506

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
62a310c
fix(exporter/otlp): env var handling
pichlermarc Aug 1, 2023
c262ced
fix(exporter/otlp): handle case where non-signal-specific endpoint en…
pichlermarc Aug 1, 2023
960c500
Merge branch 'main' into fix/otlp-exporter-env-var-handling
pichlermarc Aug 1, 2023
a8c2313
Merge branch 'main' into fix/otlp-exporter-env-var-handling
pichlermarc Aug 2, 2023
87b4137
Merge branch 'main' into fix/otlp-exporter-env-var-handling
pichlermarc Aug 3, 2023
40204f9
fix(exporter/otlp): move url logic closer to constructor
pichlermarc Aug 10, 2023
6de7fa5
Merge branch 'main' into fix/otlp-exporter-env-var-handling
robertlaurin Aug 15, 2023
c21727e
Merge branch 'main' into fix/otlp-exporter-env-var-handling
pichlermarc Sep 7, 2023
f2748b7
fix(exporter/otlp): address rubocop errors
pichlermarc Sep 7, 2023
90802ee
Merge branch 'main' into fix/otlp-exporter-env-var-handling
pichlermarc Sep 18, 2023
3a05c3d
Merge branch 'main' into fix/otlp-exporter-env-var-handling
pichlermarc Sep 20, 2023
e4d31d6
Update exporter/otlp/lib/opentelemetry/exporter/otlp/exporter.rb
pichlermarc Sep 26, 2023
316b565
Merge branch 'main' into fix/otlp-exporter-env-var-handling
mwear Oct 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 38 additions & 8 deletions exporter/otlp/lib/opentelemetry/exporter/otlp/exporter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,22 +47,16 @@ def self.ssl_verify_mode
end
end

def initialize(endpoint: OpenTelemetry::Common::Utilities.config_opt('OTEL_EXPORTER_OTLP_TRACES_ENDPOINT', 'OTEL_EXPORTER_OTLP_ENDPOINT', default: 'http://localhost:4318/v1/traces'),
def initialize(endpoint: nil,
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@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.

Copy link
Member Author

@pichlermarc pichlermarc Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to make some changes to make this pass the rubocop rules as complexity was too high f2748b7

raise ArgumentError, "unsupported compression key #{compression}" unless compression.nil? || %w[gzip none].include?(compression)

@uri = if endpoint == ENV['OTEL_EXPORTER_OTLP_ENDPOINT']
URI.join(endpoint, 'v1/traces')
else
URI(endpoint)
end

@http = http_connection(@uri, ssl_verify_mode, certificate_file)

@path = @uri.path
Expand Down Expand Up @@ -386,6 +380,42 @@ def as_otlp_any_value(value)
result
end

# Returns the url used by the exporter based on the precedence defined in the specification:
# https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#endpoint-urls-for-otlphttp
#
# @param [String, nil] endpoint
# @return [URI::Generic]
def determine_url(endpoint)
if !endpoint.nil?
# Use endpoint provided via argument as-is
parse_url_or_raise(endpoint)
elsif !ENV['OTEL_EXPORTER_OTLP_TRACES_ENDPOINT'].nil?
# Use signal-specific endpoint as-is
parse_url_or_raise(ENV['OTEL_EXPORTER_OTLP_TRACES_ENDPOINT'])
elsif !ENV['OTEL_EXPORTER_OTLP_ENDPOINT'].nil?
# Append v1/traces only if the non-signal specific env var is set.
non_signal_specific_endpoint = ENV['OTEL_EXPORTER_OTLP_ENDPOINT']
# append '/' if not present
non_signal_specific_endpoint += '/' unless non_signal_specific_endpoint.end_with?('/')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary. The URI.join call below will insert the / if required.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
else
# use default in all other cases
parse_url_or_raise('http://localhost:4318/v1/traces')
end
end

def parse_url_or_raise(endpoint)
raise ArgumentError, "invalid url for OTLP::Exporter #{endpoint}" if endpoint.nil? || endpoint.strip.empty?

URI(endpoint)
rescue URI::InvalidURIError
raise ArgumentError, "invalid url for OTLP::Exporter #{endpoint}"
end

def prepare_headers(config_headers)
headers = case config_headers
when String then parse_headers(config_headers)
Expand Down
53 changes: 51 additions & 2 deletions exporter/otlp/test/opentelemetry/exporter/otlp/exporter_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@
exp = OpenTelemetry::TestHelpers.with_env(
'OTEL_EXPORTER_OTLP_ENDPOINT' => 'https://localhost:1234/'
) do
OpenTelemetry::Exporter::OTLP::Exporter.new()
OpenTelemetry::Exporter::OTLP::Exporter.new
end
_(exp.instance_variable_get(:@path)).must_equal '/v1/traces'
end
Expand All @@ -134,11 +134,60 @@
exp = OpenTelemetry::TestHelpers.with_env(
'OTEL_EXPORTER_OTLP_ENDPOINT' => 'https://localhost:1234'
) do
OpenTelemetry::Exporter::OTLP::Exporter.new()
OpenTelemetry::Exporter::OTLP::Exporter.new
end
_(exp.instance_variable_get(:@path)).must_equal '/v1/traces'
end

it 'appends the correct path if OTEL_EXPORTER_OTLP_ENDPOINT does have a path without a trailing slash' do
exp = OpenTelemetry::TestHelpers.with_env(
# simulate OTLP endpoints built on top of an exiting API
'OTEL_EXPORTER_OTLP_ENDPOINT' => 'https://localhost:1234/api/v2/otlp'
) do
OpenTelemetry::Exporter::OTLP::Exporter.new
end
_(exp.instance_variable_get(:@path)).must_equal '/api/v2/otlp/v1/traces'
end

it 'does not join endpoint with v1/traces if endpoint is set and is equal to OTEL_EXPORTER_OTLP_ENDPOINT' do
exp = OpenTelemetry::TestHelpers.with_env(
'OTEL_EXPORTER_OTLP_ENDPOINT' => 'https://localhost:1234/custom/path'
) do
OpenTelemetry::Exporter::OTLP::Exporter.new(endpoint: 'https://localhost:1234/custom/path')
end
_(exp.instance_variable_get(:@path)).must_equal '/custom/path'
end

it 'does not append v1/traces if OTEL_EXPORTER_OTLP_ENDPOINT and OTEL_EXPORTER_OTLP_TRACES_ENDPOINT both equal' do
exp = OpenTelemetry::TestHelpers.with_env(
'OTEL_EXPORTER_OTLP_ENDPOINT' => 'https://localhost:1234/custom/path',
'OTEL_EXPORTER_OTLP_TRACES_ENDPOINT' => 'https://localhost:1234/custom/path'
) do
OpenTelemetry::Exporter::OTLP::Exporter.new
end
_(exp.instance_variable_get(:@path)).must_equal '/custom/path'
end

it 'uses OTEL_EXPORTER_OTLP_TRACES_ENDPOINT over OTEL_EXPORTER_OTLP_ENDPOINT' do
exp = OpenTelemetry::TestHelpers.with_env(
'OTEL_EXPORTER_OTLP_ENDPOINT' => 'https://localhost:1234/non/specific/custom/path',
'OTEL_EXPORTER_OTLP_TRACES_ENDPOINT' => 'https://localhost:1234/specific/custom/path'
) do
OpenTelemetry::Exporter::OTLP::Exporter.new
end
_(exp.instance_variable_get(:@path)).must_equal '/specific/custom/path'
end

it 'uses endpoint over OTEL_EXPORTER_OTLP_TRACES_ENDPOINT and OTEL_EXPORTER_OTLP_ENDPOINT' do
exp = OpenTelemetry::TestHelpers.with_env(
'OTEL_EXPORTER_OTLP_ENDPOINT' => 'https://localhost:1234/non/specific/custom/path',
'OTEL_EXPORTER_OTLP_TRACES_ENDPOINT' => 'https://localhost:1234/specific/custom/path'
) do
OpenTelemetry::Exporter::OTLP::Exporter.new(endpoint: 'https://localhost:1234/endpoint/custom/path')
end
_(exp.instance_variable_get(:@path)).must_equal '/endpoint/custom/path'
end

it 'restricts explicit headers to a String or Hash' do
exp = OpenTelemetry::Exporter::OTLP::Exporter.new(headers: { 'token' => 'über' })
_(exp.instance_variable_get(:@headers)).must_equal('token' => 'über', 'User-Agent' => DEFAULT_USER_AGENT)
Expand Down
Loading