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.

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: Explicit bucket boundary advisory parameter #1703

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
52df452
feat: Add histogram explicit_bucket_boundaries advice
kaylareopelle Aug 22, 2024
1d5c3be
Merge branch 'main' of github.com:open-telemetry/opentelemetry-ruby i…
kaylareopelle Aug 22, 2024
c728868
chore: Rubocop
kaylareopelle Aug 22, 2024
852aed5
Set default arg for advice to nil
kaylareopelle Aug 23, 2024
ebd9747
test: Move test from api to sdk
kaylareopelle Aug 23, 2024
b9e8d37
chore: Rubocop
kaylareopelle Aug 23, 2024
0a474ef
feat: Use nil instead of {} as advice default
kaylareopelle Aug 26, 2024
b723b2c
test: Update assertion
kaylareopelle Aug 28, 2024
a94e1b6
Merge branch 'main' into metrics-advisory-parameters
kaylareopelle Sep 4, 2024
7223931
test: Fix assertion deprecation warning
kaylareopelle Sep 4, 2024
ba14270
Merge branch 'metrics-advisory-parameters' of github.com:kaylareopell…
kaylareopelle Sep 4, 2024
8b56502
test: Update attributes assertion
kaylareopelle Sep 5, 2024
9e8471d
feat: Update advice default arg and ivar assign
kaylareopelle Sep 5, 2024
d2318b0
feat: Use an empty frozen hash as default advice
kaylareopelle Sep 16, 2024
ed4ceca
Revert "feat: Use an empty frozen hash as default advice"
kaylareopelle Sep 19, 2024
bcb1caf
Revert "Revert "feat: Use an empty frozen hash as default advice""
kaylareopelle Sep 19, 2024
7432bf5
Merge branch 'main' into metrics-advisory-parameters
kaylareopelle Oct 21, 2024
b50615d
chore: Rubocop trailing whitespace
kaylareopelle Oct 22, 2024
86407fd
test: Refactor new test to match data model
kaylareopelle Oct 22, 2024
7544629
Merge branch 'main' into metrics-advisory-parameters
kaylareopelle Oct 24, 2024
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
7 changes: 5 additions & 2 deletions metrics_api/lib/opentelemetry/internal/proxy_instrument.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,22 @@ module OpenTelemetry
module Internal
# @api private
class ProxyInstrument
def initialize(kind, name, unit, desc, callable)
def initialize(kind, name, unit, desc, callable, advice = nil)
kaylareopelle marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, advice is shorter than advisory_parameters, so that's why I chose that word. However, it's not the latest terminology used in the spec. I'm happy to change to advisory_parameters if that's preferred. Perhaps some documentation would clarify things.

@kind = kind
@name = name
@unit = unit
@desc = desc
@callable = callable
@advice = advice
@delegate = nil
end

def upgrade_with(meter)
@delegate = case @kind
when :counter, :histogram, :up_down_counter
when :counter, :up_down_counter
meter.send("create_#{@kind}", @name, unit: @unit, description: @desc)
when :histogram
meter.send("create_#{@kind}", @name, unit: @unit, description: @desc, advice: @advice)
when :observable_counter, :observable_gauge, :observable_up_down_counter
meter.send("create_#{@kind}", @name, unit: @unit, description: @desc, callback: @callback)
end
Expand Down
6 changes: 3 additions & 3 deletions metrics_api/lib/opentelemetry/internal/proxy_meter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,13 @@ def delegate=(meter)

private

def create_instrument(kind, name, unit, description, callback)
def create_instrument(kind, name, unit, description, callback, advice = nil)
super do
next ProxyInstrument.new(kind, name, unit, description, callback) if @delegate.nil?
next ProxyInstrument.new(kind, name, unit, description, callback, advice) if @delegate.nil?

case kind
when :counter then @delegate.create_counter(name, unit: unit, description: description)
when :histogram then @delegate.create_histogram(name, unit: unit, description: description)
when :histogram then @delegate.create_histogram(name, unit: unit, description: description, advice: advice)
when :up_down_counter then @delegate.create_up_down_counter(name, unit: unit, description: description)
when :observable_counter then @delegate.create_observable_counter(name, unit: unit, description: description, callback: callback)
when :observable_gauge then @delegate.create_observable_gauge(name, unit: unit, description: description, callback: callback)
Expand Down
6 changes: 3 additions & 3 deletions metrics_api/lib/opentelemetry/metrics/meter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ def create_counter(name, unit: nil, description: nil)
create_instrument(:counter, name, unit, description, nil) { COUNTER }
end

def create_histogram(name, unit: nil, description: nil)
create_instrument(:histogram, name, unit, description, nil) { HISTOGRAM }
def create_histogram(name, unit: nil, description: nil, advice: nil)
create_instrument(:histogram, name, unit, description, nil, advice) { HISTOGRAM }
end

def create_up_down_counter(name, unit: nil, description: nil)
Expand All @@ -55,7 +55,7 @@ def create_observable_up_down_counter(name, callback:, unit: nil, description: n

private

def create_instrument(kind, name, unit, description, callback)
def create_instrument(kind, name, unit, description, callback, advice = nil)
raise InstrumentNameError if name.nil?
raise InstrumentNameError if name.empty?
raise InstrumentNameError unless NAME_REGEX.match?(name)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ def record(amount, attributes: nil)
nil
end

def register_with_new_metric_store(metric_store, aggregation: default_aggregation)
aggregation = OpenTelemetry::SDK::Metrics::Aggregation::ExplicitBucketHistogram.new(boundaries: @advice[:explicit_bucket_boundaries]) if @advice&.key?(:explicit_bucket_boundaries)

super
end

private

def default_aggregation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@ 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, advice = {})
@name = name
@unit = unit
@description = description
@instrumentation_scope = instrumentation_scope
@meter_provider = meter_provider
@advice = advice
@metric_streams = []

meter_provider.register_synchronous_instrument(self)
Expand Down
4 changes: 2 additions & 2 deletions metrics_sdk/lib/opentelemetry/sdk/metrics/meter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,12 @@ def add_metric_reader(metric_reader)
end
end

def create_instrument(kind, name, unit, description, callback)
def create_instrument(kind, name, unit, description, callback, advice = {})
super do
case kind
when :counter then OpenTelemetry::SDK::Metrics::Instrument::Counter.new(name, unit, description, @instrumentation_scope, @meter_provider)
when :observable_counter then OpenTelemetry::SDK::Metrics::Instrument::ObservableCounter.new(name, unit, description, callback, @instrumentation_scope, @meter_provider)
when :histogram then OpenTelemetry::SDK::Metrics::Instrument::Histogram.new(name, unit, description, @instrumentation_scope, @meter_provider)
when :histogram then OpenTelemetry::SDK::Metrics::Instrument::Histogram.new(name, unit, description, @instrumentation_scope, @meter_provider, advice)
when :observable_gauge then OpenTelemetry::SDK::Metrics::Instrument::ObservableGauge.new(name, unit, description, callback, @instrumentation_scope, @meter_provider)
when :up_down_counter then OpenTelemetry::SDK::Metrics::Instrument::UpDownCounter.new(name, unit, description, @instrumentation_scope, @meter_provider)
when :observable_up_down_counter then OpenTelemetry::SDK::Metrics::Instrument::ObservableUpDownCounter.new(name, unit, description, callback, @instrumentation_scope, @meter_provider)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,29 @@
_(last_snapshot[0].data_points[0].attributes).must_equal('foo' => 'bar')
_(last_snapshot[0].aggregation_temporality).must_equal(:delta)
end

it 'can apply advice to change the explicit bucket boundaries' do
histogram = meter.create_histogram('histogram', unit: 's', description: 'test', advice: { explicit_bucket_boundaries: [5, 10, 15] })

histogram.record(0)
histogram.record(5)
histogram.record(5)
histogram.record(10)
histogram.record(20)

metric_exporter.pull
last_snapshot = metric_exporter.metric_snapshots.last
_(last_snapshot[0].data_points[0].bucket_counts).must_equal([3, 1, 0, 1])

_(last_snapshot[0].name).must_equal('histogram')
_(last_snapshot[0].unit).must_equal('s')
_(last_snapshot[0].description).must_equal('test')
_(last_snapshot[0].instrumentation_scope.name).must_equal('test')
_(last_snapshot[0].data_points[0].count).must_equal(5)
_(last_snapshot[0].data_points[0].sum).must_equal(40)
_(last_snapshot[0].data_points[0].min).must_equal(0)
_(last_snapshot[0].data_points[0].max).must_equal(20)
_(last_snapshot[0].data_points[0].attributes).must_be_nil
_(last_snapshot[0].aggregation_temporality).must_equal(:delta)
end
end
13 changes: 13 additions & 0 deletions metrics_sdk/test/opentelemetry/sdk/metrics/meter_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,19 @@
instrument = meter.create_histogram('a_histogram', unit: 'minutes', description: 'useful description')
_(instrument).must_be_instance_of OpenTelemetry::SDK::Metrics::Instrument::Histogram
end

it 'accepts advice' do
advice = { wisdom: 'this too shall pass' }
instrument = meter.create_histogram('histogram', description: 'stuff', unit: 'things', advice: advice)

_(instrument.instance_variable_get(:@advice)).must_equal(advice)
end

it 'does not require advice' do
instrument = meter.create_histogram('histogram', description: 'stuff', unit: 'things')

_(instrument.instance_variable_get(:@advice)).must_equal({})
end
end

describe '#create_up_down_counter' do
Expand Down
4 changes: 4 additions & 0 deletions metrics_sdk/test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@
require 'minitest/autorun'
require 'pry'

# The metrics test output will include an error about a missing OTLP exporter
# unless we set the traces exporter to 'none'
ENV['OTEL_TRACES_EXPORTER'] = 'none'

# reset_metrics_sdk is a test helper used to clear
# SDK configuration state between calls
def reset_metrics_sdk
Expand Down
Loading