Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def delegate=(provider)

@mutex.synchronize do
@delegate = provider
@registry.each { |key, meter| meter.delegate = provider.meter(key.name, key.version) }
@registry.each { |key, meter| meter.delegate = provider.meter(key.name, version: key.version) }
end
end

Expand All @@ -47,9 +47,9 @@ def delegate=(provider)
# @param [optional String] version Instrumentation package version
#
# @return [Meter]
def meter(name = nil, version = nil)
def meter(name = nil, version: nil)
@mutex.synchronize do
return @delegate.meter(name, version) unless @delegate.nil?
return @delegate.meter(name, version: version) unless @delegate.nil?

@registry[Key.new(name, version)] ||= ProxyMeter.new
end
Expand Down
4 changes: 2 additions & 2 deletions metrics_api/lib/opentelemetry/metrics/meter_provider.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ module Metrics
class MeterProvider
# Returns a {Meter} instance.
#
# @param [optional String] name Instrumentation package name
# @param [String] name Instrumentation package name
# @param [optional String] version Instrumentation package version
#
# @return [Meter]
def meter(name = nil, version = nil)
def meter(name, version: nil)
@meter ||= Meter.new
end
end
Expand Down
27 changes: 27 additions & 0 deletions metrics_api/test/opentelemetry/metrics/meter_provider_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# frozen_string_literal: true

# Copyright The OpenTelemetry Authors
#
# SPDX-License-Identifier: Apache-2.0

require 'test_helper'

describe OpenTelemetry::Metrics::MeterProvider do
describe '#meter' do
it 'requires a name' do
meter_provider = build_meter_provider

_(-> { meter_provider.meter }).must_raise(ArgumentError)
end

it 'returns an instance of Meter' do
meter_provider = build_meter_provider

assert(meter_provider.meter('test', version: '1.0.0').is_a?(OpenTelemetry::Metrics::Meter))
end
end

def build_meter_provider
OpenTelemetry::Metrics::MeterProvider.new
end
end
47 changes: 47 additions & 0 deletions metrics_api/test/opentelemetry/opentelemetry_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# frozen_string_literal: true

# Copyright The OpenTelemetry Authors
#
# SPDX-License-Identifier: Apache-2.0

require 'test_helper'

describe OpenTelemetry do
after do
# TODO: After Metrics SDK is incoporated into OpenTelemetry SDK, move this
# to OpenTelemetry::TestHelpers.reset_opentelemetry
OpenTelemetry.instance_variable_set(
:@meter_provider,
OpenTelemetry::Internal::ProxyMeterProvider.new
)
end

describe '#meter_provider and #meter_provider=' do
it 'initializes with a global instance of ProxyMeterProvider' do
assert(OpenTelemetry.meter_provider.is_a?(OpenTelemetry::Internal::ProxyMeterProvider))
end
Comment on lines +10 to +22
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd like to exercise that OpenTelemetry.meter_provider initially returns a ProxyMeterProvider. This test kind of exercises that, but it is not ideal. If this test runs first, then it exercises that; otherwise, it doesn't: it just exercises the reset

I was thinking if a proper reset would be calling load 'opentelemetry-metrics-api.rb to reload the file instead of manually setting @meter_provider with instance_variable_set like OpenTelemetry::TestHelpers suggests

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah it's a bit tricky if you're looking to test the initial state before any tests run. I didn't put too much importance on this because in reality all you're testing with that is that this line of code contains no syntax errors.

If this test runs first, then it exercises that; otherwise, it doesn't: it just exercises the reset

If there was an issue in this code path, it would show up as a flakey test, which is not nothing.


it 'sets global MeterProvider to the given meter_provider' do
new_meter_provider = OpenTelemetry::Metrics::MeterProvider.new

OpenTelemetry.meter_provider = new_meter_provider

assert(OpenTelemetry.meter_provider.object_id == new_meter_provider.object_id)
end

describe 'when global MeterProvider is an instance of Internal::ProxyMeterProvider' do
it 'sets ProxyMeterProvider#delegate to the given meter_provider and logs a debug message' do
proxy_meter_provider = OpenTelemetry.meter_provider
new_meter_provider = OpenTelemetry::Metrics::MeterProvider.new

OpenTelemetry::TestHelpers.with_test_logger do |log_stream|
OpenTelemetry.meter_provider = new_meter_provider

assert(proxy_meter_provider.instance_variable_get(:@delegate).object_id == new_meter_provider.object_id)
assert(OpenTelemetry.meter_provider.object_id == new_meter_provider.object_id)
assert(log_stream.string.match?(/Upgrading default proxy meter provider to #{new_meter_provider.class}/i))
end
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def update(amount, attributes)
hdp.sum += amount
hdp.count += 1
if @boundaries
bucket_index = @boundaries.bsearch_index { _1 >= amount } || @boundaries.size
bucket_index = @boundaries.bsearch_index { |i| i >= amount } || @boundaries.size
hdp.bucket_counts[bucket_index] += 1
end
nil
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def initialize(resource: OpenTelemetry::SDK::Resources::Resource.create)
# @param [optional String] version Instrumentation package version
#
# @return [Meter]
def meter(name, version = nil)
def meter(name, version: nil)
version ||= ''
if @stopped
OpenTelemetry.logger.warn 'calling MeterProvider#meter after shutdown, a noop meter will be returned.'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@
it 'invokes shutdown on all registered Metric Readers' do
mock_metric_reader1 = new_mock_reader
mock_metric_reader2 = new_mock_reader
mock_metric_reader1.expect(:shutdown, nil, [{ timeout: nil }])
mock_metric_reader2.expect(:shutdown, nil, [{ timeout: nil }])
mock_metric_reader1.expect(:shutdown, nil, [], timeout: nil)
mock_metric_reader2.expect(:shutdown, nil, [], timeout: nil)

OpenTelemetry.meter_provider.add_metric_reader(mock_metric_reader1)
OpenTelemetry.meter_provider.add_metric_reader(mock_metric_reader2)
Expand All @@ -84,8 +84,8 @@
it 'invokes force_flush on all registered Metric Readers' do
mock_metric_reader1 = new_mock_reader
mock_metric_reader2 = new_mock_reader
mock_metric_reader1.expect(:force_flush, nil, [{ timeout: nil }])
mock_metric_reader2.expect(:force_flush, nil, [{ timeout: nil }])
mock_metric_reader1.expect(:force_flush, nil, [], timeout: nil)
mock_metric_reader2.expect(:force_flush, nil, [], timeout: nil)
OpenTelemetry.meter_provider.add_metric_reader(mock_metric_reader1)
OpenTelemetry.meter_provider.add_metric_reader(mock_metric_reader2)

Expand Down