-
Notifications
You must be signed in to change notification settings - Fork 283
feat: add attributes support to instrumentation scope for tracers #2094
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,8 +9,10 @@ module SDK | |
| module Trace | ||
| # {TracerProvider} is the SDK implementation of {OpenTelemetry::Trace::TracerProvider}. | ||
| class TracerProvider < OpenTelemetry::Trace::TracerProvider # rubocop:disable Metrics/ClassLength | ||
| Key = Struct.new(:name, :version) | ||
| private_constant(:Key) | ||
| Key = Struct.new(:name, :version, :attributes) | ||
| EMPTY_ATTRIBUTES = {}.freeze | ||
|
|
||
| private_constant(:Key, :EMPTY_ATTRIBUTES) | ||
|
|
||
| attr_accessor :span_limits, :id_generator, :sampler | ||
| attr_reader :resource | ||
|
|
@@ -44,15 +46,28 @@ def initialize(sampler: sampler_from_environment(Samplers.parent_based(root: Sam | |
|
|
||
| # Returns a {Tracer} instance. | ||
| # | ||
| # @param [optional String] name Instrumentation package name | ||
| # @param [optional String] version Instrumentation package version | ||
| # Supports both positional arguments (legacy) and keyword arguments: | ||
| # tracer('name', '1.0') # legacy positional | ||
| # tracer(name: 'name', version: '1.0', attributes: {...}) # keyword | ||
| # | ||
| # When both positional and keyword arguments are provided for the same | ||
| # parameter, the keyword argument takes precedence. | ||
| # | ||
| # @param [String] name Instrumentation scope name | ||
| # @param [String] version Instrumentation scope version | ||
| # @param [Hash{String => String, Numeric, Boolean, Array<String, Numeric, Boolean>}] attributes | ||
| # Instrumentation scope attributes | ||
| # | ||
| # @return [Tracer] | ||
| def tracer(name = nil, version = nil) | ||
| name ||= '' | ||
| version ||= '' | ||
| def tracer(deprecated_name = nil, deprecated_version = nil, name: nil, version: nil, attributes: nil) # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity | ||
| name ||= deprecated_name || '' | ||
| version ||= deprecated_version || '' | ||
| attributes = attributes&.dup&.freeze || EMPTY_ATTRIBUTES | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. We should probably do attribute validation here like we do on spans. |
||
| OpenTelemetry.logger.warn 'calling TracerProvider#tracer without providing a tracer name.' if name.empty? | ||
| @registry_mutex.synchronize { @registry[Key.new(name, version)] ||= Tracer.new(name, version, self) } | ||
| @registry_mutex.synchronize do | ||
| @registry[Key.new(name, version, attributes)] ||= | ||
| Tracer.new(name, version, self, attributes: attributes) | ||
| end | ||
| end | ||
|
|
||
| # Attempts to stop all the activity for this {TracerProvider}. Calls | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -147,6 +147,7 @@ | |||||
| end | ||||||
|
|
||||||
| describe '#tracer' do | ||||||
| # Legacy positional calling conventions | ||||||
| it 'returns the same tracer for the same arguments' do | ||||||
| OpenTelemetry::TestHelpers.with_test_logger do |log_stream| | ||||||
| tracer1 = tracer_provider.tracer('component', '1.0') | ||||||
|
|
@@ -156,6 +157,11 @@ | |||||
| end | ||||||
| end | ||||||
|
|
||||||
| it 'defaults version to empty string when given positional name only' do | ||||||
| tracer = tracer_provider.tracer('component') | ||||||
| _(tracer).wont_be_nil | ||||||
| end | ||||||
|
|
||||||
| it 'returns different tracers for different names' do | ||||||
| tracer1 = tracer_provider.tracer('component1', '1.0') | ||||||
| tracer2 = tracer_provider.tracer('component2', '1.0') | ||||||
|
|
@@ -168,11 +174,68 @@ | |||||
| _(tracer1).wont_equal(tracer2) | ||||||
| end | ||||||
|
|
||||||
| it 'warn when no name is passed for the tracer' do | ||||||
| it 'warns when no name is passed for the tracer' do | ||||||
| OpenTelemetry::TestHelpers.with_test_logger do |log_stream| | ||||||
| tracer_provider.tracer | ||||||
| _(log_stream.string).must_match(/calling TracerProvider#tracer without providing a tracer name./) | ||||||
| end | ||||||
| end | ||||||
|
|
||||||
| # Keyword calling conventions | ||||||
| it 'accepts all keyword arguments' do | ||||||
| tracer = tracer_provider.tracer(name: 'component', version: '1.0', attributes: { 'key' => 'value' }) | ||||||
| _(tracer).wont_be_nil | ||||||
| end | ||||||
|
|
||||||
| it 'returns the same tracer for equivalent positional and keyword arguments' do | ||||||
| tracer1 = tracer_provider.tracer('component', '1.0') | ||||||
| tracer2 = tracer_provider.tracer(name: 'component', version: '1.0') | ||||||
| _(tracer1).must_equal(tracer2) | ||||||
| end | ||||||
|
|
||||||
| # Mixed positional + keyword | ||||||
| it 'accepts positional name with keyword version' do | ||||||
| tracer = tracer_provider.tracer('component', version: '1.0') | ||||||
| _(tracer).wont_be_nil | ||||||
| end | ||||||
|
|
||||||
| it 'prefers keyword arguments over positional arguments' do | ||||||
| tracer1 = tracer_provider.tracer('positional', '1.0') | ||||||
| tracer2 = tracer_provider.tracer('keyword', '2.0', name: 'keyword', version: '2.0') | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔
Suggested change
I think these suggested |
||||||
| _(tracer1).wont_equal(tracer2) | ||||||
| end | ||||||
|
|
||||||
| # Attributes | ||||||
| it 'returns the same tracer for the same name, version, and attributes' do | ||||||
| tracer1 = tracer_provider.tracer('component', '1.0', attributes: { 'key' => 'value' }) | ||||||
| tracer2 = tracer_provider.tracer('component', '1.0', attributes: { 'key' => 'value' }) | ||||||
| _(tracer1).must_equal(tracer2) | ||||||
| end | ||||||
|
|
||||||
| it 'returns different tracers for different attributes' do | ||||||
| tracer1 = tracer_provider.tracer('component', '1.0', attributes: { 'key' => 'value1' }) | ||||||
| tracer2 = tracer_provider.tracer('component', '1.0', attributes: { 'key' => 'value2' }) | ||||||
| _(tracer1).wont_equal(tracer2) | ||||||
| end | ||||||
|
|
||||||
| it 'returns different tracers for attributes vs no attributes' do | ||||||
| tracer1 = tracer_provider.tracer('component', '1.0') | ||||||
| tracer2 = tracer_provider.tracer('component', '1.0', attributes: { 'key' => 'value' }) | ||||||
| _(tracer1).wont_equal(tracer2) | ||||||
| end | ||||||
|
|
||||||
| it 'treats nil attributes the same as no attributes' do | ||||||
| tracer1 = tracer_provider.tracer('component', '1.0') | ||||||
| tracer2 = tracer_provider.tracer('component', '1.0', attributes: nil) | ||||||
| _(tracer1).must_equal(tracer2) | ||||||
| end | ||||||
|
|
||||||
| it 'does not allow mutation of attributes after tracer creation' do | ||||||
| attrs = { 'key' => 'value' } | ||||||
| tracer_provider.tracer('component', '1.0', attributes: attrs) | ||||||
| attrs['key'] = 'mutated' | ||||||
| tracer = tracer_provider.tracer('component', '1.0', attributes: { 'key' => 'value' }) | ||||||
| _(tracer).wont_be_nil | ||||||
| end | ||||||
| end | ||||||
| end | ||||||
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.
Rather than have conditional code executed on every
provider.tracer(...)call, could we conditionally wrap theproviderbefore the mutex with a legacy provider that just drops theattributes:arg? I.e. (roughly)This would avoid penalizing providers implementing the new signature (which should be the common case, assuming most people use the SDK provider) and avoids the conditional dispatch in the legacy case.