-
Notifications
You must be signed in to change notification settings - Fork 252
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
feat: advisory parameters for metrics instruments #1801
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 |
---|---|---|
|
@@ -24,11 +24,21 @@ | |
_(counter.class).must_equal(OpenTelemetry::Metrics::Instrument::Counter) | ||
end | ||
|
||
it 'test create_counter with advisory parameters' do | ||
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. Considering tests as documentation, I think it would be helpful to add assertions for the other instruments with their permitted advisory parameters. |
||
counter = meter.create_counter('test', attributes: { 'test' => true }) | ||
_(counter.class).must_equal(OpenTelemetry::Metrics::Instrument::Counter) | ||
end | ||
|
||
it 'test create_histogram' do | ||
counter = meter.create_histogram('test') | ||
_(counter.class).must_equal(OpenTelemetry::Metrics::Instrument::Histogram) | ||
end | ||
|
||
it 'test create_histogram with advisory parameters' do | ||
histogram = meter.create_histogram('test', explicit_bucket_boundaries: [1, 2, 3]) | ||
_(histogram.class).must_equal(OpenTelemetry::Metrics::Instrument::Histogram) | ||
end | ||
|
||
it 'test create_up_down_counter' do | ||
counter = meter.create_up_down_counter('test') | ||
_(counter.class).must_equal(OpenTelemetry::Metrics::Instrument::UpDownCounter) | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -35,7 +35,21 @@ def record(amount, attributes: {}) | |||||
private | ||||||
|
||||||
def default_aggregation | ||||||
OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new | ||||||
# This hash is assembled to avoid implicitly passing `boundaries: nil`, | ||||||
# which should be valid explicit call according to ExplicitBucketHistogram#initialize | ||||||
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
|
||||||
kwargs = {} | ||||||
kwargs[:attributes] = @attributes if @attributes | ||||||
kwargs[:boundaries] = @boundaries if @boundaries | ||||||
|
||||||
OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new(**kwargs) | ||||||
end | ||||||
|
||||||
def validate_advisory_parameters(parameters) | ||||||
if (boundaries = parameters.delete(:explicit_bucket_boundaries)) | ||||||
@boundaries = boundaries | ||||||
end | ||||||
|
||||||
super | ||||||
end | ||||||
end | ||||||
end | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,19 +11,21 @@ module Instrument | |
# {SynchronousInstrument} contains the common functionality shared across | ||
# the synchronous instruments SDK instruments. | ||
class SynchronousInstrument | ||
def initialize(name, unit, description, instrumentation_scope, meter_provider) | ||
def initialize(name, unit, description, instrumentation_scope, meter_provider, **advisory_parameters) | ||
@name = name | ||
@unit = unit | ||
@description = description | ||
@instrumentation_scope = instrumentation_scope | ||
@meter_provider = meter_provider | ||
@metric_streams = [] | ||
|
||
validate_advisory_parameters(advisory_parameters) | ||
|
||
meter_provider.register_synchronous_instrument(self) | ||
end | ||
|
||
# @api private | ||
def register_with_new_metric_store(metric_store, aggregation: default_aggregation) | ||
def register_with_new_metric_store(metric_store) | ||
ms = OpenTelemetry::SDK::Metrics::State::MetricStream.new( | ||
@name, | ||
@description, | ||
|
@@ -37,11 +39,25 @@ def register_with_new_metric_store(metric_store, aggregation: default_aggregatio | |
metric_store.add_metric_stream(ms) | ||
end | ||
|
||
def aggregation | ||
@aggregation || default_aggregation | ||
end | ||
|
||
private | ||
|
||
def update(value, attributes) | ||
@metric_streams.each { |ms| ms.update(value, attributes) } | ||
end | ||
|
||
def validate_advisory_parameters(advisory_parameters) | ||
if (attributes = advisory_parameters.delete(:attributes)) | ||
@attributes = attributes | ||
end | ||
|
||
advisory_parameters.each_key do |parameter_name| | ||
OpenTelemetry.logger.warn "Advisory parameter `#{parameter_name}` is not valid for instrument kind `#{instrument_kind}`; ignoring" | ||
end | ||
Comment on lines
+57
to
+59
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. Would you mind adding a test that covers this scenario? |
||
end | ||
end | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,4 +30,30 @@ | |
_(last_snapshot[0].data_points[0].attributes).must_equal('foo' => 'bar') | ||
_(last_snapshot[0].aggregation_temporality).must_equal(:delta) | ||
end | ||
|
||
describe 'with advisory parameters' do | ||
let(:random_key) { "a#{SecureRandom.hex}" } | ||
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. The tests on 3.0 are failing with a missing SecureRandom require: https://github.com/open-telemetry/opentelemetry-ruby/actions/runs/13058885253/job/36668636949?pr=1801#step:6:185 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. Also, similar question to the API -- what do you think about adding advisory parameters/attributes for the other instruments? |
||
let(:counter) do | ||
meter.create_counter( | ||
'counter', | ||
unit: 'smidgen', | ||
description: 'a small amount of something', | ||
attributes: { random_key => true } | ||
) | ||
end | ||
|
||
it 'counts' do | ||
counter.add(1, attributes: { 'foo' => 'bar' }) | ||
metric_exporter.pull | ||
last_snapshot = metric_exporter.metric_snapshots | ||
|
||
_(last_snapshot[0].name).must_equal('counter') | ||
_(last_snapshot[0].unit).must_equal('smidgen') | ||
_(last_snapshot[0].description).must_equal('a small amount of something') | ||
_(last_snapshot[0].instrumentation_scope.name).must_equal('test') | ||
_(last_snapshot[0].data_points[0].value).must_equal(1) | ||
_(last_snapshot[0].data_points[0].attributes).must_equal('foo' => 'bar', random_key => true) | ||
_(last_snapshot[0].aggregation_temporality).must_equal(:delta) | ||
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.
Could you add some documentation to each of the instruments about the relevant
**advisory_parameters
named keyword args?