feat: Rework Meter and MeterProvider API #1442

elias19r · 2023-04-27T00:55:15Z

Remove argument validations from API and add doc comments:
validation should be performed by the implementation of the API
Add new parent classes SynchronousInstrument and AsynchronousInstrument
Make name required for Meter
Add prefix NOOP_ to no-op constant instruments
Normalize Instrument names with downcase before registering them
because their name should be case-insensitive
Add advice to Synchronous Instruments

elias19r · 2023-05-12T18:31:13Z

metrics_api/.rubocop.yml

+
+Naming/VariableNumber:
+  Enabled: false


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

elias19r · 2023-05-12T18:33:18Z

metrics_api/.rubocop.yml

+
+Style/IfUnlessModifier:
+  Enabled: false


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

elias19r · 2023-05-12T18:37:21Z

metrics_api/lib/opentelemetry/metrics/meter.rb

-        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))


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?

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.

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

elias19r · 2023-05-12T18:40:47Z

metrics_api/lib/opentelemetry/metrics/meter.rb

-        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))
+        name = name.downcase


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]

elias19r · 2023-05-12T19:02:07Z

metrics_api/lib/opentelemetry/metrics/instrument/asynchronous_instrument.rb

+        # @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


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

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

robertlaurin · 2023-05-23T15:21:49Z

metrics_api/lib/opentelemetry/metrics/instrument/synchronous_instrument.rb

+        attr_reader :name, :unit, :description
+
+        # @api private
+        def initialize(name, unit: nil, description: nil)


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

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

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

metrics_api/lib/opentelemetry/metrics/meter.rb

robertlaurin · 2023-05-23T15:30:07Z

metrics_api/lib/opentelemetry/metrics/meter.rb

-        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))


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.

Co-authored-by: Robert <[email protected]>

…thoughtbot/opentelemetry-ruby into elias/meter-and-instrument-creation-api

elias19r · 2023-05-26T20:43:33Z

If you squash&merge, feel free to use the PR's description in the commit message (and add/edit anything). Otherwise, let me know and I can re-create commits with better messages :D

…er_callback}

robertlaurin · 2023-06-02T14:53:20Z

@elias19r I'm about ready to merge, there's some branch conflicts that need to be resolved first though.

fbogsany · 2023-06-02T19:32:37Z

metrics_api/lib/opentelemetry/metrics/instrument/asynchronous_instrument.rb

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


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

Good catch! I think they changed it. https://opentelemetry.io/docs/reference/specification/metrics/api/#asynchronous-instrument-api is redirecting to https://opentelemetry.io/docs/specs/otel/metrics/api/#asynchronous-instrument-api with status 301. I'll update them!

Updated in e4cd5c2

fbogsany · 2023-06-02T19:34:21Z

metrics_api/lib/opentelemetry/metrics/instrument/asynchronous_instrument.rb

+          @name = name
+          @unit = unit || ''
+          @description = description || ''
+          @callback = callback ? Array(callback) : []


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

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

fbogsany · 2023-06-02T19:37:26Z

metrics_api/lib/opentelemetry/metrics/instrument/asynchronous_instrument.rb

+        #   - 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); end


Similarly, the plural form is preferable. I'd also like to use the "splat" form:

Suggested change

def register_callback(callback); end

def register_callbacks(*callback); end

This allows calling as:

instr.register_callbacks(cb1) instr.register_callbacks(cb2, cb3)

instead of

instr.register_callback(cb1) instr.register_callback([cb2, cb3])

I like the splat form, but I've been thinking if should actually support all those cases later in the implementation:

instr.register_callback(cb1) instr.register_callback(cb2, cb3) instr.register_callback([cb4, cb5]) # And support this as well? # Just so it is consistent with `callbacks:` from the `Meter#create_observable_counter` API

In the implementation, I think we could support that with

def register_callbacks(*callbacks) @callbacks.concat(callbacks.flatten) end

Because otherwise it would feel odd to be able to pass an array of procs when creating an instrument, like

meter = MeterProvider.meter('test') instr = meter.create_observable_counter('test', callbacks: [cb1, cb2])

but not be able to pass an array of procs when registering callbacks later

meter = MeterProvider.meter('test') instr = meter.create_observable_counter('test') instr.register_callbacks([cb1, cb2]) # ??

Let me know what you think! Updated in 9d9ad67

fbogsany · 2023-06-05T15:36:12Z

metrics_api/test/opentelemetry/metrics/instrument/asynchronous_instrument_test.rb

+
+      assert(instrument.register_callbacks(-> {}).nil?)
+      assert(instrument.register_callbacks(-> {}, -> {}).nil?)
+      assert(instrument.register_callbacks([-> {}, -> {}]).nil?)


Suggested change

assert(instrument.register_callbacks([-> {}, -> {}]).nil?)

assert(instrument.register_callbacks(-> {}, -> {}).nil?)

What do you think of allowing all these use cases:

instr.register_callback(cb1) instr.register_callback(cb2, cb3) instr.register_callback([cb4, cb5]) # And support this as well? # Just so it is consistent with `callbacks:` from the `Meter#create_observable_counter` API

See #1442 (comment)

fbogsany · 2023-06-05T15:36:22Z

metrics_api/test/opentelemetry/metrics/instrument/asynchronous_instrument_test.rb

+
+      assert(instrument.unregister_callbacks(-> {}).nil?)
+      assert(instrument.unregister_callbacks(-> {}, -> {}).nil?)
+      assert(instrument.unregister_callbacks([-> {}, -> {}]).nil?)


Suggested change

assert(instrument.unregister_callbacks([-> {}, -> {}]).nil?)

assert(instrument.unregister_callbacks(-> {}, -> {}).nil?)

fbogsany · 2023-06-05T15:38:31Z

metrics_api/lib/opentelemetry/metrics/instrument/asynchronous_instrument.rb

+          @callbacks = callbacks ? Array(callbacks) : []
+        end
+
+        # @param callbacks [Proc, Array<Proc>]


Technically, callbacks is an Array<Proc> now, always - I'm not sure how to document a "splat" param.

fbogsany · 2023-06-05T15:38:42Z

metrics_api/lib/opentelemetry/metrics/instrument/asynchronous_instrument.rb

+        #     across all registered callbacks;
+        def register_callbacks(*callbacks); end
+
+        # @param callbacks [Proc, Array<Proc>]


fbogsany · 2023-06-05T15:45:46Z

metrics_api/lib/opentelemetry/metrics/meter.rb

+      # @param advice [optional Hash] Set of recommendations aimed at assisting
+      #   implementations in providing useful output with minimal configuration


Do we have a better description of advice? I'm working through the spec, and I haven't found anything useful yet. I have no idea how to use it - Hash is very generic - what are allowed key & value types?

I would have expected advice to be a simple String, with set here not meaning an actual data structure, but I feel like I'm missing a lot of context.

It seems advice has been added to the specification recently. I'm not sure how it will be exactly used. For now, I'm thinking of it as an "options" Hash with recommended values. For example in docs, the recommended bucket boundaries for a histogram. I imagine they will define standard names to be used in advice, maybe:

advice: { histogram: { explicit_bucket_boundaries: [0.1, 0.5, 1.0, 5.0, 10.0, 25.0, Float::INFINITY] } }

Reading the SDK page, I found another reference to advice in Default Aggregation:

The Default Aggregation informs the SDK to use the Instrument kind to select an aggregation and advice to influence aggregation configuration parameters (as noted in the "Selected Aggregation" column).

Instrument Kind Selected Aggregation

[...] [...]

Histogram Explicit Bucket Histogram Aggregation, with ExplicitBucketBoundaries from advice if provided

I don't think this should block the current PR from being merged, but it seems that advice has been renamed to advisory parameters.

API: https://opentelemetry.io/docs/specs/otel/metrics/api/#instrument-advisory-parameters

SDK: https://opentelemetry.io/docs/specs/otel/metrics/sdk/#instrument-advisory-parameters

It has a mixed status in the API and is experimental in the SDK.

elias19r · 2023-08-14T19:21:49Z

@robertlaurin @fbogsany Hi, I haven't had a lot of time to work on this, but I could start a follow-up PR adjusting the existing SDK code to the changes I'm making to the API here.

thoughtbot#1
(I've opened it in another repo just for visibility. I'll move to here if this PR gets merged)

It is mainly about making the Proxy Instruments and SDK Instruments inherit from the API Instrument classes. If you get a chance to review it, let me know if that makes sense :)

After that, I wonder if we could complete the implementation of one instrument (maybe Counter). I don't quite see the big picture yet, but I'm starting to understand the concepts of the data model for metrics.

Hronom · 2023-09-17T01:29:36Z

We interested in getting suport for metrics at least no auto instrumentation for ruby, it's shame that it still not have is maintainers can implement it?

…trument-creation-api

elias19r · 2023-12-08T22:07:37Z

I think I won't be able to keep up with those PRs, it's been a while since I read the docs. I'll close them but keep the branches in case the changes are useful.

feat: Rework Meter and MeterProvider API

58ba96a

elias19r mentioned this pull request May 10, 2023

test: Add coverage for other create instrument methods #1440

Closed

elias19r added 2 commits May 11, 2023 15:35

WIP

c419345

Merge branch 'main' into elias/meter-and-instrument-creation-api

ad6fdd1

elias19r commented May 12, 2023

View reviewed changes

elias19r marked this pull request as ready for review May 12, 2023 18:42

elias19r requested review from fbogsany, mwear, robertlaurin, dazuma, ericmustin, arielvalentin, ahayworth, plantfansam and robbkidd as code owners May 12, 2023 18:42

elias19r commented May 12, 2023

View reviewed changes

robbkidd added this to the Metrics v1.0 milestone May 16, 2023

elias19r added 2 commits May 19, 2023 13:13

Merge branch 'main' into elias/meter-and-instrument-creation-api

b2c0e2c

Merge branch 'main' into elias/meter-and-instrument-creation-api

a678314

robertlaurin reviewed May 23, 2023

View reviewed changes

elias19r and others added 5 commits May 26, 2023 15:53

Merge branch 'main' into elias/meter-and-instrument-creation-api

d666a90

Fix yard doc comment for meter attributes

11906b1

Fix typo in no-op instrument names

b9db252

Co-authored-by: Robert <[email protected]>

Merge branch 'elias/meter-and-instrument-creation-api' of github.com:…

d79e525

…thoughtbot/opentelemetry-ruby into elias/meter-and-instrument-creation-api

Add advice to synchronous instruments

7cce415

Remove bodies from AsynchronousInstrument#{register_callback,unregist…

b0ba487

…er_callback}

robertlaurin approved these changes Jun 2, 2023

View reviewed changes

Merge branch 'main' into elias/meter-and-instrument-creation-api

bda5259

fbogsany reviewed Jun 2, 2023

View reviewed changes

elias19r added 4 commits June 2, 2023 16:46

Update comments with links to Metrics API specification

e4cd5c2

Pluralize callback --> callbacks

443d35e

Use splat from in register_callbacks

9d9ad67

Fixup comment

f96741b

fbogsany reviewed Jun 5, 2023

View reviewed changes

Merge branch 'main' into elias/meter-and-instrument-creation-api

6af99b7

kaylareopelle mentioned this pull request Jun 27, 2023

TruffleRuby Test Exporter test failling #1476

Open

Merge branch 'main' into elias/meter-and-instrument-creation-api

671b5c0

elias19r requested a review from fbogsany July 1, 2023 20:42

elias19r mentioned this pull request Jul 7, 2023

Adjust existing Metrics SDK code to match changes to API thoughtbot/opentelemetry-ruby#1

Closed

Merge branch 'main' into elias/meter-and-instrument-creation-api

f12cc09

Merge branch 'main' into elias/meter-and-instrument-creation-api

787790b

elias19r added 4 commits September 25, 2023 12:37

Merge branch 'main' into elias/meter-and-instrument-creation-api

703faf3

Merge branch 'main' into elias/meter-and-instrument-creation-api

d919961

Merge branch 'main' into elias/meter-and-instrument-creation-api

dde3ee8

Merge remote-tracking branch 'upstream/main' into elias/meter-and-ins…

1f4641c

…trument-creation-api

elias19r closed this Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Rework Meter and MeterProvider API #1442

feat: Rework Meter and MeterProvider API #1442

elias19r commented Apr 27, 2023 •

edited

Loading

elias19r May 12, 2023 •

edited

Loading

elias19r May 12, 2023 •

edited

Loading

elias19r May 12, 2023

robertlaurin May 23, 2023

elias19r May 26, 2023

elias19r May 12, 2023

elias19r May 12, 2023 •

edited

Loading

elias19r May 26, 2023 •

edited

Loading

robertlaurin May 23, 2023

elias19r May 26, 2023

elias19r May 26, 2023

robertlaurin May 23, 2023

elias19r commented May 26, 2023

robertlaurin commented Jun 2, 2023

fbogsany Jun 2, 2023

elias19r Jun 2, 2023 •

edited

Loading

fbogsany Jun 2, 2023

elias19r Jun 2, 2023 •

edited

Loading

fbogsany Jun 2, 2023 •

edited

Loading

elias19r Jun 2, 2023 •

edited

Loading

fbogsany Jun 5, 2023

elias19r Jun 5, 2023

fbogsany Jun 5, 2023

fbogsany Jun 5, 2023

fbogsany Jun 5, 2023

fbogsany Jun 5, 2023

elias19r Jun 5, 2023

elias19r Jul 1, 2023

kaylareopelle Dec 7, 2023

elias19r commented Aug 14, 2023

Hronom commented Sep 17, 2023

elias19r commented Dec 8, 2023

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

	def register_callback(callback); end
	def register_callbacks(*callback); end

	assert(instrument.register_callbacks([-> {}, -> {}]).nil?)
	assert(instrument.register_callbacks(-> {}, -> {}).nil?)

	assert(instrument.unregister_callbacks([-> {}, -> {}]).nil?)
	assert(instrument.unregister_callbacks(-> {}, -> {}).nil?)

		# @param advice [optional Hash] Set of recommendations aimed at assisting
		# implementations in providing useful output with minimal configuration

Instrument Kind	Selected Aggregation
[...]	[...]
Histogram	Explicit Bucket Histogram Aggregation, with `ExplicitBucketBoundaries` from `advice` if provided

feat: Rework Meter and MeterProvider API #1442

feat: Rework Meter and MeterProvider API #1442

Conversation

elias19r commented Apr 27, 2023 • edited Loading

elias19r May 12, 2023 • edited Loading

Choose a reason for hiding this comment

elias19r May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elias19r May 12, 2023 • edited Loading

Choose a reason for hiding this comment

elias19r May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elias19r commented May 26, 2023

robertlaurin commented Jun 2, 2023

Choose a reason for hiding this comment

elias19r Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elias19r Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

fbogsany Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

elias19r Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elias19r commented Aug 14, 2023

Hronom commented Sep 17, 2023

elias19r commented Dec 8, 2023

elias19r commented Apr 27, 2023 •

edited

Loading

elias19r May 12, 2023 •

edited

Loading

elias19r May 12, 2023 •

edited

Loading

elias19r May 12, 2023 •

edited

Loading

elias19r May 26, 2023 •

edited

Loading

elias19r Jun 2, 2023 •

edited

Loading

elias19r Jun 2, 2023 •

edited

Loading

fbogsany Jun 2, 2023 •

edited

Loading

elias19r Jun 2, 2023 •

edited

Loading