Metrics SDK synchronous instruments #1108

robertlaurin · 2022-01-27T16:05:04Z

First pass implementation of the Metrics SDK.

In scope:

Synchronous instruments
- Counter
- Up down counter
- Histogram
Default aggregation
- Sum
- (optional) ExplicitBucketHistogram
Meter Provider provisioning
Meter provisioning
Synchronous instrument creation

Out of scope:

Revisions of the API to reflect spec churn
Views
Observable instruments
Exporters (otlp, prom, etc)
Metric Readers (periodic pull reader)
Aggregation configuration
- Aggregation temporality
- Exponential bucket histogram aggregation, etc...

metrics_api/lib/opentelemetry-metrics-api.rb

metrics_sdk/lib/opentelemetry/metrics_sdk.rb

metrics_sdk/lib/opentelemetry/sdk/metrics/meter.rb

metrics_sdk/test/opentelemetry/metrics_sdk_test.rb

fbogsany · 2022-02-23T14:29:54Z

metrics_sdk/lib/opentelemetry/sdk/metrics/export/console_exporter.rb

+            SUCCESS
+          end
+
+          def preferred_temporality


Looks like we also need a way to specify aggregation temporality - i.e. an attribute writer or an initializer argument.

Will differ configuration of temporality to a follow up PR.

fbogsany · 2022-02-23T14:31:29Z

metrics_sdk/lib/opentelemetry/sdk/metrics/export/metric_reader.rb

+            @exporter = exporter
+          end
+
+          def collect; end


We might want an optional timeout parameter here as well. Not sure yet.

fbogsany · 2022-02-23T14:36:08Z

metrics_sdk/lib/opentelemetry/sdk/metrics/meter_provider.rb

+              return
+            end
+
+            @metric_readers = @metric_readers.push(metric_reader)


Don't really need the assignment here, since push mutates the receiver.

Suggested change

@metric_readers = @metric_readers.push(metric_reader)

@metric_readers.push(metric_reader)

Dumb question: this will return the @metric_readers, right? Is that a good idea or should we explicitly return nil? I know we do the same in TracerProvider - I think that might be a mistake as well.

fbogsany · 2022-02-23T14:49:19Z

metrics_sdk/lib/opentelemetry/sdk/metrics/meter_provider.rb

+        #   a non-specific failure occurred, Export::TIMEOUT if a timeout occurred.
+        def force_flush(timeout: nil)
+          @mutex.synchronize do
+            return Export::SUCCESS if @stopped


I'm curious about the style difference between this and shutdown above, specifically returning from the block here, and not in shutdown. Poking around, it looks like MRI will ensure the mutex is released even on return, though this isn't explicitly specified in the docs. We use return in the TracerProvider as well.

I think it's just a stylistic difference. Do you have a preference around the use of explicit vs implicit returns for a method like this? (I guess the argument could be made that we should just remain consistent with the style already being used in the tracer provider)

fbogsany · 2022-02-23T14:50:48Z

metrics_sdk/lib/opentelemetry/sdk/metrics/meter_provider.rb

+        # Attempts to stop all the activity for this {MeterProvider}.
+        #
+        # Calls MetricReader#shutdown for all registered MetricReaders.
+        # Calls MetricExporter#shutdown for all registered MetricExporters.


We're not doing anything with MetricExporters yet.

metrics_sdk/lib/opentelemetry/sdk/metrics/meter_provider.rb

yfractal · 2022-05-19T02:45:16Z

Hi everyone, do we have plan to support this?

robertlaurin · 2022-05-19T14:12:18Z

Hi everyone, do we have plan to support this?

Hey @yfractal, could you clarify what you mean by support this? This is a WIP branch for the metrics SDK implementation, when it's no longer a WIP we will release and support it if that is what you are asking.

metrics_sdk/lib/opentelemetry/sdk/metrics/export.rb

fbogsany · 2022-05-19T20:55:36Z

metrics_sdk/lib/opentelemetry/sdk/metrics/export/console_metric_exporter.rb

+    module Metrics
+      module Export
+        class ConsoleMetricExporter
+          PREFERRED_TEMPORALITY = 'delta'


Since we're using frozen_string_literal, is there any benefit to the named constant?

yfractal · 2022-05-20T02:17:08Z

Hi @robertlaurin, yeah. it is, so great! hope it will be merged into master soon.

I'm working on Ruby OpenTelemetry metric related thing, like to help on this.

robertlaurin · 2022-05-20T18:35:41Z

Hi @robertlaurin, yeah. it is, so great! hope it will be merged into master soon.

I'm working on Ruby OpenTelemetry metric related thing, like to help on this.

Definitely want this to be out sooner than later too 😄

Right now the focus is getting the foundational pieces of metrics SDK implemented to make it easier for collaboration on it. So an non exhaustive list would be:

Metrics sdk initialization
Meter creation
Synchronous instrument creation
Synchronous metric state storage
Some basic metric readers/exporters (console, in memory)

Once those are at a point where we feel like there is some stability this PR will ideally get merged.

Some of the obvious work to pick up after that will be:

Various exporters (prometheus for example)
Adding observable instruments (async instruments)
Adding support for views
Many iterations of spec compliance review

yfractal · 2022-05-24T06:27:58Z

Right now the focus is getting the foundational pieces of metrics SDK implemented to make it easier for collaboration on it.

I read through the specification and Python SDK those days, I fond there are a lot of concepts and not easy to understand.

If we have the "foundational pieces", it will be help a lot !

robertlaurin · 2022-06-28T20:10:26Z

@ahayworth @fbogsany this is at reviewable stopping point again.

metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation.rb

fbogsany · 2022-06-29T20:46:45Z

metrics_sdk/lib/opentelemetry/sdk/metrics/export/metric_reader.rb

+      module Export
+        # MetricReader provides a minimal example implementation.
+        # It is not required to subclass this class to provide an implementation
+        # of MetricReader, provided the interface is satisfied.


I think it's problematic that we have to expose metric_store as part of the interface. That should be a SDK-internal concern, which means we probably do have to mark it as API-private and require concrete implementations to subclass this class.

I agree, I was really hoping to provide a simple duck type interface similar to the span processor instead of requiring it them to inherit from a base metric reader.

The intention is to have more focused work around this be part of the follow up PRs for things like the periodic pull reader exporter.

metrics_sdk/lib/opentelemetry/sdk/metrics/export/in_memory_metric_pull_exporter.rb

fbogsany · 2022-06-29T21:00:33Z

metrics_sdk/lib/opentelemetry/sdk/metrics/state/metric_stream.rb

+            @mutex = Mutex.new
+          end
+
+          def collect(start_time, end_time)


Does data_points keep growing without bound? I'm not clear on the expected behaviour here - part of me thinks we're supposed to only return data points recorded during the interval start_time .. end_time, but I'm really not sure.

That needs to be addressed as part of the aggregation temporality work that is not part of this PR, i.e. sum or delta.

In the case of sum, there should be some concept of pruning. It's mentioned in the guidelines.

So here are some suggestions that we encourage SDK implementers to consider:

You want to control the memory usage rather than allow it to grow indefinitely / unbounded - regardless of what aggregation temporality is being used.

You want to improve the memory efficiency by being able to forget about things that are no longer needed.

You probably don't want to keep exporting the same thing over and over again, if there is no updates. You might want to consider Resets and Gaps. For example, if a Cumulative metrics stream hasn't received any updates for a long period of time, would it be okay to reset the start time?

ahayworth

I don't have much to add other than what Francis already mentioned!

metrics_sdk/lib/opentelemetry/sdk/metrics/instrument/counter.rb

Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>

Co-authored-by: Andrew Hayworth <ahayworth@gmail.com>

plantfansam

I have a few questions — feel free to disregard, as most are borne from not fully understanding the spec. Nice work!

plantfansam · 2022-06-30T19:51:44Z

metrics_api/lib/opentelemetry/metrics/meter.rb


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


Might consider including something like "this can result in corrupted / invalid metrics" or whatever, since that's a potential consequence.

Will get addressed in future revisions/reviews of the API, a few things changed since we merged the original API PR.

plantfansam · 2022-06-30T19:55:17Z

metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation.rb

+      module Aggregation
+        extend self
+
+        SUM = ->(v1, v2) { v1 + v2 }


Maybe worth a comment as to why we're setting a constant to a lambda, here (related: why are we setting a constant to a lambda, here?)

Reduce allocations, similar idea to

opentelemetry-ruby/api/lib/opentelemetry/baggage/propagation.rb

Lines 17 to 25 in a46ba22

TEXT_MAP_PROPAGATOR = TextMapPropagator.new

private_constant :TEXT_MAP_PROPAGATOR

# Returns a text map propagator that propagates context using the

# W3C Baggage format.

def text_map_propagator

TEXT_MAP_PROPAGATOR

end

This bit is likely to change in a follow up PR I'm working on.

plantfansam · 2022-06-30T19:58:29Z

metrics_sdk/lib/opentelemetry/sdk/metrics/configuration_patch.rb

+    module Metrics
+      # The ConfiguratorPatch implements a hook to configure the metrics
+      # portion of the SDK.
+      module ConfiguratorPatch


Should the file be called configurator_patch.rb or this class be called ConfigurationPatch or is misalignment intentional?

Good catch, the name misalignment is not intentional. I'll fix it in my follow up PR.

plantfansam · 2022-06-30T19:59:58Z

metrics_sdk/lib/opentelemetry/sdk/metrics/configuration_patch.rb

+        # The metrics_configuration_hook method is where we define the setup process
+        # for metrics SDK.
+        def metrics_configuration_hook
+          OpenTelemetry.meter_provider = Metrics::MeterProvider.new(resource: @resource)


Perhaps worth a comment indicating that @resource instance var should be avail in the Configurator, but 🤷

The @resource instance var will be available in the configurator, it's a little awkward as were developing this as a separate gem, but once we have stabilized the metrics api/sdk it will get merged into the SDK/API and this patch stuff will go away.

plantfansam · 2022-06-30T20:10:30Z

metrics_sdk/lib/opentelemetry/sdk/metrics/instrument/histogram.rb

+          #   Array values must not contain nil elements and all elements must be of
+          #   the same basic type (string, numeric, boolean).
+          def record(amount, attributes: nil)
+            update(OpenTelemetry::Metrics::Measurement.new(amount, attributes))


Do we want to warn if negative numeric value, here?

The histogram aggregation needs to be added properly, I'm working on that in a separate branch to follow up on this one.

plantfansam · 2022-06-30T20:31:28Z

metrics_sdk/lib/opentelemetry/sdk/metrics/meter_provider.rb

+        def shutdown(timeout: nil)
+          @mutex.synchronize do
+            if @stopped
+              OpenTelemetry.logger.warn('calling MetricProvider#shutdown multiple times.')


Will this always be true? If not, perhaps clearer to say "calling MetricProvider#shutdown while MeterProvider is marked as @stopped"?

plantfansam · 2022-06-30T20:40:45Z

metrics_sdk/lib/opentelemetry/sdk/metrics/state/metric_store.rb

+        class MetricStore
+          def initialize
+            @mutex = Mutex.new
+            @epoch_start_time = now_in_nano


An epoch from start_time to end_time is basically the duration between calls to collect, is that right?

plantfansam · 2022-06-30T20:41:18Z

metrics_sdk/lib/opentelemetry/sdk/metrics/state/metric_store.rb

+
+          def add_metric_stream(metric_stream)
+            @mutex.synchronize do
+              @metric_streams = @metric_streams.dup.push(metric_stream)


Why .dup?

It's a means of making the change atomic, the @metric_streams value won't change mid process. It'll either have the old array, or the new one. It won't change "in flight" so to speak.

plantfansam · 2022-06-30T20:45:22Z

metrics_sdk/lib/opentelemetry/sdk/metrics/state/metric_stream.rb

+
+          def collect(start_time, end_time)
+            @mutex.synchronize do
+              MetricData.new(


I'm not clear on how instantiating this object fulfills the obligations of collect, but I'm definitely missing something....

Think of it as collecting a snapshot of the metrics at a certain point in time. The metrics will accumulate between collect calls, so we want to capture all the information necessary to export and the current state of the data points. What's not shown here is that we will also need to configure the temporality of the metrics, so in the case of delta, after grabbing this snapshot we will drop all the datapoints. In cumulative temporality we need the datapoints and their values at the time of collection, but after collection finishes the data points on the metric stream can continue to grow, but we don't want to have that data change once it has been sent off for export. This is intended to be a simple struct to encapsulate this information distinctly from the metric stream.

plantfansam · 2022-06-30T20:49:50Z

metrics_sdk/lib/opentelemetry/sdk/metrics/meter.rb

+
+        def create_instrument(kind, name, unit, description, callback)
+          super do
+            case kind


The intent is that super do will yield the return of the case statement to the superclass?

plantfansam · 2022-07-06T14:25:08Z

Hey merging this is a huge achievement! Way to go @robertlaurin!

lisamburns · 2022-07-26T00:03:50Z

This is exciting! Would love to use it, when will be a release cut?

ahayworth · 2022-07-26T01:36:49Z

Hi @lisamburns - not in the immediate future. We need to add aggregation support in progress here, and then exporter support (not yet formally started, although very rough versions have been written as proof-of-concept stuff).

I don't know that we have a specific timeline to share, but @robertlaurin has been working on getting it to a state where the work can be done in parallel. He could likely share more information!

robertlaurin · 2022-07-28T18:30:23Z

Hey @lisamburns, the metrics isn't quite ready for consumption yet.

We'll cut a release when more of the implementation is complete. In its current state you could simple create synchronous instruments but the export path is not implemented, and the aggregation is incomplete.

You can track the work in flight and the work to do on the metrics project board https://github.com/open-telemetry/opentelemetry-ruby/projects/2

fbogsany reviewed Jan 27, 2022

View reviewed changes

metrics_api/lib/opentelemetry-metrics-api.rb Outdated Show resolved Hide resolved

robertlaurin self-assigned this Jan 27, 2022

fbogsany reviewed Jan 27, 2022

View reviewed changes

metrics_sdk/lib/opentelemetry/metrics_sdk.rb Outdated Show resolved Hide resolved

Base automatically changed from new-metrics-api to main January 27, 2022 17:19

robertlaurin force-pushed the new-metrics-sdk branch from 433deeb to 8404617 Compare January 27, 2022 17:23

fbogsany mentioned this pull request Jan 27, 2022

SDK: implement metrics when OTEPs are approved #81

Closed

fbogsany reviewed Feb 10, 2022

View reviewed changes

metrics_sdk/lib/opentelemetry/sdk/metrics/meter.rb Outdated Show resolved Hide resolved

fbogsany reviewed Feb 10, 2022

View reviewed changes

metrics_sdk/lib/opentelemetry/sdk/metrics/meter.rb Outdated Show resolved Hide resolved

fbogsany reviewed Feb 10, 2022

View reviewed changes

metrics_sdk/lib/opentelemetry/sdk/metrics/meter.rb Outdated Show resolved Hide resolved

fbogsany reviewed Feb 10, 2022

View reviewed changes

metrics_sdk/test/opentelemetry/metrics_sdk_test.rb Outdated Show resolved Hide resolved

robertlaurin force-pushed the new-metrics-sdk branch from aa18e8a to f64082e Compare February 22, 2022 20:54

fbogsany reviewed Feb 23, 2022

View reviewed changes

metrics_sdk/lib/opentelemetry/sdk/metrics/meter_provider.rb Show resolved Hide resolved

robertlaurin force-pushed the new-metrics-sdk branch from 418b7d7 to f075de5 Compare February 25, 2022 19:58

robertlaurin force-pushed the new-metrics-sdk branch from c78ee10 to 99d64ca Compare May 10, 2022 01:32

fbogsany reviewed May 19, 2022

View reviewed changes

metrics_sdk/lib/opentelemetry/sdk/metrics/export.rb Show resolved Hide resolved

fbogsany reviewed May 19, 2022

View reviewed changes

robertlaurin added 4 commits June 20, 2022 16:09

WIP: Metrics SDK implementation

935ef1e

Extend the existing OpenTelemetry SDK

ac267b2

fix: remove duplicate mutex instantiation

2b2a176

feat: add api metrics measurement

62959df

robertlaurin added 6 commits June 28, 2022 13:57

feedback fixups

fdfda89

Remove console exporter

79605fd

Add integration test assertions

dfcd8f5

fix: broken test assert

e8684d6

chore: appease the cop

cd4fbb7

chore: appease the cop

a46ba22

Merge branch 'main' into new-metrics-sdk

ffb20a4

fbogsany reviewed Jun 29, 2022

View reviewed changes

metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation.rb Show resolved Hide resolved

fbogsany reviewed Jun 29, 2022

View reviewed changes

metrics_sdk/lib/opentelemetry/sdk/metrics/export/in_memory_metric_pull_exporter.rb Outdated Show resolved Hide resolved

fbogsany reviewed Jun 29, 2022

View reviewed changes

ahayworth approved these changes Jun 30, 2022

View reviewed changes

metrics_sdk/lib/opentelemetry/sdk/metrics/instrument/counter.rb Show resolved Hide resolved

robertlaurin and others added 2 commits June 30, 2022 10:46

fix: francis feedback

241db2c

Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>

fix: add hayworth comment

c261b6d

Co-authored-by: Andrew Hayworth <ahayworth@gmail.com>

robertlaurin marked this pull request as ready for review June 30, 2022 15:51

robertlaurin requested review from mwear, dazuma, ericmustin, arielvalentin and plantfansam as code owners June 30, 2022 15:51

robertlaurin changed the title ~~WIP: Metrics SDK implementation~~ Metrics SDK synchronous instruments Jun 30, 2022

plantfansam reviewed Jun 30, 2022

View reviewed changes

robertlaurin merged commit 6727391 into main Jul 5, 2022

robertlaurin deleted the new-metrics-sdk branch July 5, 2022 19:22

	@metric_readers = @metric_readers.push(metric_reader)
	@metric_readers.push(metric_reader)

	TEXT_MAP_PROPAGATOR = TextMapPropagator.new

	private_constant :TEXT_MAP_PROPAGATOR

	# Returns a text map propagator that propagates context using the
	# W3C Baggage format.
	def text_map_propagator
	TEXT_MAP_PROPAGATOR
	end

Metrics SDK synchronous instruments #1108

Metrics SDK synchronous instruments #1108

Conversation

robertlaurin commented Jan 27, 2022 • 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

yfractal commented May 19, 2022

robertlaurin commented May 19, 2022

Choose a reason for hiding this comment

yfractal commented May 20, 2022 • edited Loading

robertlaurin commented May 20, 2022

yfractal commented May 24, 2022

robertlaurin commented Jun 28, 2022

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

ahayworth left a comment

Choose a reason for hiding this comment

plantfansam left a 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

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

plantfansam commented Jul 6, 2022

lisamburns commented Jul 26, 2022

ahayworth commented Jul 26, 2022

robertlaurin commented Jul 28, 2022

robertlaurin commented Jan 27, 2022 •

edited

Loading

yfractal commented May 20, 2022 •

edited

Loading