feat: advisory parameters for metrics instruments#1801
feat: advisory parameters for metrics instruments#1801zvkemp wants to merge 1 commit intoopen-telemetry:mainfrom
Conversation
zvkemp
requested review from
ahayworth,
arielvalentin,
dazuma,
ericmustin,
fbogsany,
kaylareopelle,
mwear,
plantfansam,
robbkidd and
robertlaurin
as code owners
zvkemp
force-pushed
the
metrics-kwargs
branch
from
c2cb6cf to
eade680
Compare
zvkemp
force-pushed
the
metrics-kwargs
branch
from
eade680 to
8811940
Compare
|
Example of this in use in the Redis PR |
kaylareopelle
left a comment
kaylareopelle
left a comment
There was a problem hiding this comment.
Thanks, @zvkemp! This looks great. 🎉 I have a few testing/documentation recommendations, but the logic is sound and aligns with the spec.
When we make changes to the API/SDK, I also think about whether any changes need to be made to the OTLP exporter. In this case, it seems like the OTLP exporter should accept these attributes without any changes. A test might be helpful down the line to confirm explicit bucket boundaries are passed along, but that can be handled in a separate unit of work.
| end | ||
|
|
||
| describe 'with advisory parameters' do | ||
| let(:random_key) { "a#{SecureRandom.hex}" } |
There was a problem hiding this comment.
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.
Also, similar question to the API -- what do you think about adding advisory parameters/attributes for the other instruments?
| ) | ||
| end | ||
|
|
||
| let(:random_attribute) { "a#{SecureRandom.hex}" } |
There was a problem hiding this comment.
Same comment as above -- tests are failing with a missing SecureRandom require: https://github.com/open-telemetry/opentelemetry-ruby/actions/runs/13058885253/job/36668636949?pr=1801#step:6:185
| # @return [nil] after creation of counter, it will be stored in instrument_registry | ||
| def create_counter(name, unit: nil, description: nil) | ||
| create_instrument(:counter, name, unit, description, nil) { COUNTER } | ||
| def create_counter(name, unit: nil, description: nil, **advisory_parameters) |
There was a problem hiding this comment.
Could you add some documentation to each of the instruments about the relevant **advisory_parameters named keyword args?
| _(counter.class).must_equal(OpenTelemetry::Metrics::Instrument::Counter) | ||
| end | ||
|
|
||
| it 'test create_counter with advisory parameters' do |
There was a problem hiding this comment.
Considering tests as documentation, I think it would be helpful to add assertions for the other instruments with their permitted advisory parameters.
| 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.
| # which should be valid explicit call according to ExplicitBucketHistogram#initialize | |
| # which should be a valid explicit call according to ExplicitBucketHistogram#initialize |
| advisory_parameters.each_key do |parameter_name| | ||
| OpenTelemetry.logger.warn "Advisory parameter `#{parameter_name}` is not valid for instrument kind `#{instrument_kind}`; ignoring" | ||
| end |
There was a problem hiding this comment.
Would you mind adding a test that covers this scenario?
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
2 participants
According to the metrics API, instrument creation should support advisory parameters.
The two current parameters are
attributesandexplicit_bucket_boundaries.attributes(status: development), apply to every instrument.explicit_bucket_boundariesonly apply to histograms.The advisory parameters are accepted at instrument creation, validated that they apply to that instrument type, and then are cached on the instrument. New instances of their configured aggregations receive these parameters at
#initialize.