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: Rework Meter and MeterProvider API #1442

Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
58ba96a
feat: Rework Meter and MeterProvider API
elias19r Apr 26, 2023
c419345
WIP
elias19r May 11, 2023
ad6fdd1
Merge branch 'main' into elias/meter-and-instrument-creation-api
elias19r May 12, 2023
b2c0e2c
Merge branch 'main' into elias/meter-and-instrument-creation-api
elias19r May 19, 2023
a678314
Merge branch 'main' into elias/meter-and-instrument-creation-api
elias19r May 23, 2023
d666a90
Merge branch 'main' into elias/meter-and-instrument-creation-api
elias19r May 26, 2023
11906b1
Fix yard doc comment for meter attributes
elias19r May 26, 2023
b9db252
Fix typo in no-op instrument names
elias19r May 26, 2023
d79e525
Merge branch 'elias/meter-and-instrument-creation-api' of github.com:…
elias19r May 26, 2023
7cce415
Add advice to synchronous instruments
elias19r May 26, 2023
b0ba487
Remove bodies from AsynchronousInstrument#{register_callback,unregist…
elias19r May 26, 2023
bda5259
Merge branch 'main' into elias/meter-and-instrument-creation-api
elias19r Jun 2, 2023
e4cd5c2
Update comments with links to Metrics API specification
elias19r Jun 2, 2023
443d35e
Pluralize callback --> callbacks
elias19r Jun 2, 2023
9d9ad67
Use splat from in register_callbacks
elias19r Jun 2, 2023
f96741b
Fixup comment
elias19r Jun 2, 2023
6af99b7
Merge branch 'main' into elias/meter-and-instrument-creation-api
elias19r Jun 17, 2023
671b5c0
Merge branch 'main' into elias/meter-and-instrument-creation-api
elias19r Jun 30, 2023
f12cc09
Merge branch 'main' into elias/meter-and-instrument-creation-api
elias19r Aug 11, 2023
787790b
Merge branch 'main' into elias/meter-and-instrument-creation-api
elias19r Sep 1, 2023
703faf3
Merge branch 'main' into elias/meter-and-instrument-creation-api
elias19r Sep 25, 2023
d919961
Merge branch 'main' into elias/meter-and-instrument-creation-api
elias19r Oct 11, 2023
dde3ee8
Merge branch 'main' into elias/meter-and-instrument-creation-api
elias19r Nov 20, 2023
1f4641c
Merge remote-tracking branch 'upstream/main' into elias/meter-and-ins…
elias19r Dec 8, 2023
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: 7 additions & 0 deletions metrics_api/.rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ AllCops:

Lint/UnusedMethodArgument:
Enabled: false

Metrics/AbcSize:
Enabled: false
Metrics/LineLength:
Expand All @@ -15,8 +16,14 @@ Metrics/CyclomaticComplexity:
Max: 20
Metrics/ParameterLists:
Enabled: false

Naming/VariableNumber:
Enabled: false
Comment on lines +21 to +23
Copy link
Contributor Author

@elias19r elias19r May 12, 2023

Choose a reason for hiding this comment

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

I'm happy to revert any of these style changes, just let me know

nit: For this one, I think that naming variables with something_1 and something_2 in tests is easier to read

Naming/FileName:
Exclude:
- "lib/opentelemetry-metrics-api.rb"

Style/IfUnlessModifier:
Enabled: false
Comment on lines +27 to +29
Copy link
Contributor Author

@elias19r elias19r May 12, 2023

Choose a reason for hiding this comment

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

nit: When this is enabled, we can be forced into writing long lines with if/unless at the end which I think may not be easy to read

Style/ModuleFunction:
Enabled: false
5 changes: 4 additions & 1 deletion metrics_api/lib/opentelemetry/metrics/instrument.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@
#
# SPDX-License-Identifier: Apache-2.0

require 'opentelemetry/metrics/instrument/synchronous_instrument'
require 'opentelemetry/metrics/instrument/counter'
require 'opentelemetry/metrics/instrument/histogram'
require 'opentelemetry/metrics/instrument/up_down_counter'

require 'opentelemetry/metrics/instrument/asynchronous_instrument'
require 'opentelemetry/metrics/instrument/observable_counter'
require 'opentelemetry/metrics/instrument/observable_gauge'
require 'opentelemetry/metrics/instrument/observable_up_down_counter'
require 'opentelemetry/metrics/instrument/up_down_counter'

module OpenTelemetry
module Metrics
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# frozen_string_literal: true

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

module OpenTelemetry
module Metrics
module Instrument
# https://opentelemetry.io/docs/reference/specification/metrics/api/#asynchronous-instrument-api
Copy link
Contributor

Choose a reason for hiding this comment

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

This link didn't work for me. I found the doc at:

Suggested change
# https://opentelemetry.io/docs/reference/specification/metrics/api/#asynchronous-instrument-api
# https://opentelemetry.io/docs/specs/otel/metrics/api/#asynchronous-instrument-api

Copy link
Contributor Author

@elias19r elias19r Jun 2, 2023

Choose a reason for hiding this comment

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

class AsynchronousInstrument
attr_reader :name, :unit, :description, :callback

# @api private
def initialize(name, unit: nil, description: nil, callback: nil)
@name = name
@unit = unit || ''
@description = description || ''
@callback = callback ? Array(callback) : []
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: since this is an array of functions, can we pluralize the name as callbacks?

Copy link
Contributor Author

@elias19r elias19r Jun 2, 2023

Choose a reason for hiding this comment

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

Sure, I can pluralize them!

I think the reason I initially didn't do that was to keep the same "symbol same" from the specification although this @callback instance variable is internal.

But in Meter API, in metrics_api/lib/opentelemetry/metrics/meter.rb, should I pluralize the keyword arg callback: as well in the create_observable_counter, create_observable_gauge, and create_observable_up_down_counter methods?

Updated in 443d35e

end

# @param callback [Proc, Array<Proc>]
# Callback functions should:
# - be reentrant safe;
# - not take an indefinite amount of time;
# - not make duplicate observations (more than one Measurement with the same attributes)
# across all registered callbacks;
def register_callback(callback)
@callback.concat(Array(callback))
end

# @param callback [Proc, Array<Proc>]
# Callback functions should:
# - be reentrant safe;
# - not take an indefinite amount of time;
# - not make duplicate observations (more than one Measurement with the same attributes)
# across all registered callbacks;
def unregister_callback(callback)
@callback -= Array(callback)
end
Copy link
Contributor Author

@elias19r elias19r May 12, 2023

Choose a reason for hiding this comment

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

This is how I imagine register/unregister callback functions would look like in Ruby. Feel free to suggest any changes

https://opentelemetry.io/docs/specs/otel/metrics/api/#asynchronous-instrument-api

Copy link
Contributor Author

@elias19r elias19r May 26, 2023

Choose a reason for hiding this comment

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

I just realized that, with this, people are able to register callbacks to No-Op instances of Asynchronous Instruments. I think I should delete these methods' bodies and leave them to be implemented by the SDK

Updated in b0ba487

end
end
end
end
4 changes: 2 additions & 2 deletions metrics_api/lib/opentelemetry/metrics/instrument/counter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ module OpenTelemetry
module Metrics
module Instrument
# No-op implementation of Counter.
class Counter
class Counter < SynchronousInstrument
# Increment the Counter by a fixed amount.
#
# @param [numeric] increment The increment amount, which MUST be a non-negative numeric value.
# @param [Numeric] increment The increment amount, which MUST be a non-negative numeric value.
# @param [Hash{String => String, Numeric, Boolean, Array<String, Numeric, Boolean>}] attributes
# Values must be non-nil and (array of) string, boolean or numeric type.
# Array values must not contain nil elements and all elements must be of
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ module OpenTelemetry
module Metrics
module Instrument
# No-op implementation of Histogram.
class Histogram
class Histogram < SynchronousInstrument
# Updates the statistics with the specified amount.
#
# @param [numeric] amount The amount of the Measurement, which MUST be a non-negative numeric value.
# @param [Numeric] amount The amount of the Measurement, which MUST be a non-negative numeric value.
# @param [Hash{String => String, Numeric, Boolean, Array<String, Numeric, Boolean>}] attributes
# Values must be non-nil and (array of) string, boolean or numeric type.
# Array values must not contain nil elements and all elements must be of
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ module OpenTelemetry
module Metrics
module Instrument
# No-op implementation of ObservableCounter.
class ObservableCounter
# TODO
class ObservableCounter < AsynchronousInstrument
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ module OpenTelemetry
module Metrics
module Instrument
# No-op implementation of ObservableGauge.
class ObservableGauge
# TODO
class ObservableGauge < AsynchronousInstrument
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ module OpenTelemetry
module Metrics
module Instrument
# No-op implementation of ObservableUpDownCounter.
class ObservableUpDownCounter
# TODO
class ObservableUpDownCounter < AsynchronousInstrument
end
end
end
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# frozen_string_literal: true

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

module OpenTelemetry
module Metrics
module Instrument
# https://opentelemetry.io/docs/reference/specification/metrics/api/#synchronous-instrument-api
class SynchronousInstrument
attr_reader :name, :unit, :description

# @api private
def initialize(name, unit: nil, description: nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • advice for implementations.

Users can provide advice, but its up to their discretion. Therefore, this API needs to be structured to accept advice, but MUST NOT obligate the user to provide it.

advice needs to be structured as described in instrument advice, with parameters that are general and specific to a particular instrument kind. The API SHOULD NOT validate advice.

Did you want to work this in as a follow up PR? https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#synchronous-instrument-api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that advice has been added recently. I can include it in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 7cce415

I'm not sure how advice will be exactly used, but I think of it as an "options" Hash with recommended values. For example in docs, the recommended bucket boundaries for a histogram. I imagine they will add standard names to be used in advice

@name = name
@unit = unit || ''
@description = description || ''
end
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module OpenTelemetry
module Metrics
module Instrument
# No-op implementation of UpDownCounter.
class UpDownCounter
class UpDownCounter < SynchronousInstrument
# Increment or decrement the UpDownCounter by a fixed amount.
#
# @param [Numeric] amount The amount to be added, can be positive, negative or zero.
Expand Down
157 changes: 121 additions & 36 deletions metrics_api/lib/opentelemetry/metrics/meter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,71 +8,156 @@ module OpenTelemetry
module Metrics
# No-op implementation of Meter.
class Meter
COUNTER = Instrument::Counter.new
OBSERVABLE_COUNTER = Instrument::ObservableCounter.new
HISTOGRAM = Instrument::Histogram.new
OBSERVABLE_GAUGE = Instrument::ObservableGauge.new
UP_DOWN_COUNTER = Instrument::UpDownCounter.new
OBSERVABLE_UP_DOWN_COUNTER = Instrument::ObservableUpDownCounter.new
NOOP_COUNTER = Instrument::Counter.new('np-op')
NOOP_HISTOGRAM = Instrument::Histogram.new('np-op')
NOOP_UP_DOWN_COUNTER = Instrument::UpDownCounter.new('np-op')
NOOP_OBSERVABLE_COUNTER = Instrument::ObservableCounter.new('np-op')
NOOP_OBSERVABLE_GAUGE = Instrument::ObservableGauge.new('np-op')
NOOP_OBSERVABLE_UP_DOWN_COUNTER = Instrument::ObservableUpDownCounter.new('np-op')
elias19r marked this conversation as resolved.
Show resolved Hide resolved

private_constant(
:NOOP_COUNTER,
:NOOP_HISTOGRAM,
:NOOP_UP_DOWN_COUNTER,
:NOOP_OBSERVABLE_COUNTER,
:NOOP_OBSERVABLE_GAUGE,
:NOOP_OBSERVABLE_UP_DOWN_COUNTER
)

attr_reader :name, :version, :schema_url, :attributes

# @api private
def initialize(name, version: nil, schema_url: nil, attributes: nil)
@name = name
@version = version || ''
@schema_url = schema_url || ''
@attributes = attributes || {}

NAME_REGEX = /^[a-zA-Z][-.\w]{0,62}$/.freeze

private_constant(:COUNTER, :OBSERVABLE_COUNTER, :HISTOGRAM, :OBSERVABLE_GAUGE, :UP_DOWN_COUNTER, :OBSERVABLE_UP_DOWN_COUNTER)

DuplicateInstrumentError = Class.new(OpenTelemetry::Error)
InstrumentNameError = Class.new(OpenTelemetry::Error)
InstrumentUnitError = Class.new(OpenTelemetry::Error)
InstrumentDescriptionError = Class.new(OpenTelemetry::Error)

def initialize
@mutex = Mutex.new
@instrument_registry = {}
end

# @param name [String]
# Must conform to the instrument name syntax:
# not nil or empty, case-insensitive ASCII string that matches `/\A[a-z][a-z0-9_.-]{0,62}\z/i`
# @param unit [optional String]
# Must conform to the instrument unit rule:
# case-sensitive ASCII string with maximum length of 63 characters
# @param description [optional String]
# Must conform to the instrument description rule:
# UTF-8 string but up to 3 bytes per charater with maximum length of 1023 characters
#
# @return [Instrument::Counter]
def create_counter(name, unit: nil, description: nil)
create_instrument(:counter, name, unit, description, nil) { COUNTER }
create_instrument(:counter, name, unit, description, nil) { NOOP_COUNTER }
end

# @param name [String]
# Must conform to the instrument name syntax:
# not nil or empty, case-insensitive ASCII string that matches `/\A[a-z][a-z0-9_.-]{0,62}\z/i`
# @param unit [optional String]
# Must conform to the instrument unit rule:
# case-sensitive ASCII string with maximum length of 63 characters
# @param description [optional String]
# Must conform to the instrument description rule:
# UTF-8 string but up to 3 bytes per charater with maximum length of 1023 characters
#
# @return [Instrument::Histogram]
def create_histogram(name, unit: nil, description: nil)
create_instrument(:histogram, name, unit, description, nil) { HISTOGRAM }
create_instrument(:histogram, name, unit, description, nil) { NOOP_HISTOGRAM }
end

# @param name [String]
# Must conform to the instrument name syntax:
# not nil or empty, case-insensitive ASCII string that matches `/\A[a-z][a-z0-9_.-]{0,62}\z/i`
# @param unit [optional String]
# Must conform to the instrument unit rule:
# case-sensitive ASCII string with maximum length of 63 characters
# @param description [optional String]
# Must conform to the instrument description rule:
# UTF-8 string but up to 3 bytes per charater with maximum length of 1023 characters
#
# @return [Instrument::UpDownCounter]
def create_up_down_counter(name, unit: nil, description: nil)
create_instrument(:up_down_counter, name, unit, description, nil) { UP_DOWN_COUNTER }
create_instrument(:up_down_counter, name, unit, description, nil) { NOOP_UP_DOWN_COUNTER }
end

def create_observable_counter(name, unit: nil, description: nil, callback:)
create_instrument(:observable_counter, name, unit, description, callback) { OBSERVABLE_COUNTER }
# @param name [String]
# Must conform to the instrument name syntax:
# not nil or empty, case-insensitive ASCII string that matches `/\A[a-z][a-z0-9_.-]{0,62}\z/i`
# @param unit [optional String]
# Must conform to the instrument unit rule:
# case-sensitive ASCII string with maximum length of 63 characters
# @param description [optional String]
# Must conform to the instrument description rule:
# UTF-8 string but up to 3 bytes per charater with maximum length of 1023 characters
# @param callback [optional Proc, Array<Proc>]
# Callback functions should:
# - be reentrant safe;
# - not take an indefinite amount of time;
# - not make duplicate observations (more than one Measurement with the same attributes)
# across all registered callbacks;
#
# @return [Instrument::ObservableCounter]
def create_observable_counter(name, unit: nil, description: nil, callback: nil)
create_instrument(:observable_counter, name, unit, description, callback) { NOOP_OBSERVABLE_COUNTER }
end

def create_observable_gauge(name, unit: nil, description: nil, callback:)
create_instrument(:observable_gauge, name, unit, description, callback) { OBSERVABLE_GAUGE }
# @param name [String]
# Must conform to the instrument name syntax:
# not nil or empty, case-insensitive ASCII string that matches `/\A[a-z][a-z0-9_.-]{0,62}\z/i`
# @param unit [optional String]
# Must conform to the instrument unit rule:
# case-sensitive ASCII string with maximum length of 63 characters
# @param description [optional String]
# Must conform to the instrument description rule:
# UTF-8 string but up to 3 bytes per charater with maximum length of 1023 characters
# @param callback [optional Proc, Array<Proc>]
# Callback functions should:
# - be reentrant safe;
# - not take an indefinite amount of time;
# - not make duplicate observations (more than one Measurement with the same attributes)
# across all registered callbacks;
#
# @return [Instrument::ObservableGauge]
def create_observable_gauge(name, unit: nil, description: nil, callback: nil)
create_instrument(:observable_gauge, name, unit, description, callback) { NOOP_OBSERVABLE_GAUGE }
end

def create_observable_up_down_counter(name, unit: nil, description: nil, callback:)
create_instrument(:observable_up_down_counter, name, unit, description, callback) { OBSERVABLE_UP_DOWN_COUNTER }
# @param name [String]
# Must conform to the instrument name syntax:
# not nil or empty, case-insensitive ASCII string that matches `/\A[a-z][a-z0-9_.-]{0,62}\z/i`
# @param unit [optional String]
# Must conform to the instrument unit rule:
# case-sensitive ASCII string with maximum length of 63 characters
# @param description [optional String]
# Must conform to the instrument description rule:
# UTF-8 string but up to 3 bytes per charater with maximum length of 1023 characters
# @param callback [optional Proc, Array<Proc>]
# Callback functions should:
# - be reentrant safe;
# - not take an indefinite amount of time;
# - not make duplicate observations (more than one Measurement with the same attributes)
# across all registered callbacks;
#
# @return [Instrument::ObservableUpDownCounter]
def create_observable_up_down_counter(name, unit: nil, description: nil, callback: nil)
create_instrument(:observable_up_down_counter, name, unit, description, callback) { NOOP_OBSERVABLE_UP_DOWN_COUNTER }
end

private

def create_instrument(kind, name, unit, description, callback)
raise InstrumentNameError if name.nil?
raise InstrumentNameError if name.empty?
raise InstrumentNameError unless NAME_REGEX.match?(name)
raise InstrumentUnitError if unit && (!unit.ascii_only? || unit.size > 63)
raise InstrumentDescriptionError if description && (description.size > 1023 || !utf8mb3_encoding?(description.dup))
Comment on lines -59 to -63
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After reading Synchronous Instrument API I noticed the following

The API SHOULD be documented in a way to communicate to users that the
name parameter needs to conform to the instrument name syntax.
The API SHOULD NOT validate the name; that is left to
implementations of the API.

and

The unit parameter needs to support the instrument unit rule.
Meaning, the API MUST accept a case-sensitive string that supports
ASCII character encoding and can hold at least 63 characters.
The API SHOULD NOT validate the unit.

Similar for name and unit in Asynchronous Instrument API

So, I think we should remove these validations from here and add yard doc comments instead. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

So, I think we should remove these validations from here and add yard doc comments instead. What do you think?

I think that's reasonable, the API spec is explicit in that it should perform any validations against the name field, so that will be the responsibility of the SDK implementation. Yard docs seem like a good solution for this.

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 in the "Metrics No-Op API Implementation" docs it makes clear No-Op Meters must not validate. For example:

The Meter MUST accept these parameters. However, the Meter MUST NOT validate any argument it receives.

https://opentelemetry.io/docs/specs/otel/metrics/noop/#counter-creation

name = name.downcase
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is new. From the Metrics' Meter API specification for the Instrument Name Syntax it says:

They are case-insensitive, ASCII strings.

So here I'm normalizing the name with downcase before using it in @instrument_registry[name]


@mutex.synchronize do
OpenTelemetry.logger.warn("duplicate instrument registration occurred for instrument #{name}") if @instrument_registry.include? name
if @instrument_registry.include?(name)
OpenTelemetry.logger.warn("duplicate instrument registration occurred for #{name}")
end

@instrument_registry[name] = yield
end
end

def utf8mb3_encoding?(string)
string.force_encoding('UTF-8').valid_encoding? &&
string.each_char { |c| return false if c.bytesize >= 4 }
end
end
end
end
Loading
Loading