Adjust existing Metrics SDK code to match changes to API #1

elias19r · 2023-07-07T13:40:33Z

This PR is just for visibility.

The goal is to open this PR against main in open-telemetry/opentelemetry-ruby
after PR #1442 is merged.

Make Proxy Instrument classes inherit from API Instrument classes
Make SDK Instrument classes inherit from API Instrument classes
Add kind method to Instruments
Make Instruments have a reference to Meter
Make Meter have a reference to MeterProvider
Make Meter responsible for creating a MetricStream for an Instrument
and MetricStore and for registering new instruments with existing MetricStreams
Add empty Aggregation::LastValue
Make Meter hold the definition of default aggregation for Instruments
Make Aggregation::Sum test cases more verbose with examples
Move InstrumentationScope creation to Meter on initialize
Add schema_url and attributes to InstrumentationScope (?)

…ault See https://opentelemetry.io/docs/specs/otel/resource/sdk/

See https://opentelemetry.io/docs/specs/otel/metrics/sdk/#meter-creation

See https://opentelemetry.io/docs/specs/otel/metrics/api/#get-a-meter > Meters are identified by `name`, `version`, and `schema_url` fields.

And instrument_kind methods

…ust-existing-metrics-sdk-code-to-match-api

And I need to make the last test case work again. I thought we could avoid making Meter hold a reference to MeterProvider but that's no possible due to add_metric_reader

and Instruments hold a reference to Meter

… created

Also, - Remove .dup that I _think_ is not needed - Rename snapshot variable to metric_data

elias19r · 2023-08-13T20:55:29Z

metrics_api/lib/opentelemetry/internal/proxy_instrument.rb

+  module Metrics
+    module ProxyInstrument
    end
  end
 end
+
+require 'opentelemetry/internal/proxy_instrument/delegate_synchronous_instrument'
+require 'opentelemetry/internal/proxy_instrument/counter'
+require 'opentelemetry/internal/proxy_instrument/histogram'
+require 'opentelemetry/internal/proxy_instrument/up_down_counter'
+
+require 'opentelemetry/internal/proxy_instrument/delegate_asynchronous_instrument'
+require 'opentelemetry/internal/proxy_instrument/observable_counter'
+require 'opentelemetry/internal/proxy_instrument/observable_gauge'
+require 'opentelemetry/internal/proxy_instrument/observable_up_down_counter'


In order to make Proxy Instruments inherit from API Instrument classes, I've created a Proxy Instrument class for each Instrument. They include the delegate= setter from a module.

elias19r · 2023-08-13T20:59:16Z

metrics_api/lib/opentelemetry/internal/proxy_meter.rb

-        @mutex.synchronize do
+        @delegate_mutex.synchronize do
          if @delegate.nil?
            @delegate = meter
-            @instrument_registry.each_value { |instrument| instrument.upgrade_with(meter) }
+
+            @instrument_registry.each_value do |proxy_instrument|
+              proxy_instrument.delegate =
+                case proxy_instrument
+                when ProxyInstrument::Counter
+                  meter.create_counter(
+                    proxy_instrument.name,
+                    unit:        proxy_instrument.unit,
+                    description: proxy_instrument.description,
+                    advice:      proxy_instrument.advice
+                  )
+                when ProxyInstrument::Histogram
+                  meter.create_histogram(
+                    proxy_instrument.name,
+                    unit:        proxy_instrument.unit,
+                    description: proxy_instrument.description,
+                    advice:      proxy_instrument.advice
+                  )
+                when ProxyInstrument::UpDownCounter
+                  meter.create_up_down_counter(
+                    proxy_instrument.name,
+                    unit:        proxy_instrument.unit,
+                    description: proxy_instrument.description,
+                    advice:      proxy_instrument.advice
+                  )
+                when ProxyInstrument::ObservableCounter
+                  meter.create_observable_counter(
+                    proxy_instrument.name,
+                    unit:        proxy_instrument.unit,
+                    description: proxy_instrument.description,
+                    callbacks:   proxy_instrument.callbacks
+                  )
+                when ProxyInstrument::ObservableGauge
+                  meter.create_observable_gauge(
+                    proxy_instrument.name,
+                    unit:        proxy_instrument.unit,
+                    description: proxy_instrument.description,
+                    callbacks:   proxy_instrument.callbacks
+                  )
+                when ProxyInstrument::ObservableUpDownCounter
+                  meter.create_observable_up_down_counter(
+                    proxy_instrument.name,
+                    unit:        proxy_instrument.unit,
+                    description: proxy_instrument.description,
+                    callbacks:   proxy_instrument.callbacks
+                  )
+                end
+            end


This is more verbose now because we have a Proxy Instrument class for each Instrument

elias19r · 2023-08-13T21:04:03Z

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

+        # @api private
+        def add_metric_stream(metric_stream)
+          @mutex.synchronize do
+            @metric_streams.push(metric_stream)
+            nil
+          end
+        end
+
+        private
+
+        def update(value, attributes)
+          @mutex.synchronize do
+            @metric_streams.each do |metric_stream|
+              metric_stream.update(value, attributes)
+            end
+          end
        end


Moved add_metric_reader and update to the API Instrument class.

The logic to create a MetricStream is in the Meter

elias19r · 2023-08-13T21:06:00Z

metrics_api/lib/opentelemetry/metrics/meter.rb

+      def register_instrument(name, instrument)
        name = name.downcase

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

-          @instrument_registry[name] = yield
+          @instrument_registry[name] = instrument
        end
      end


Renamed create_instrument to register_instrument for clarity and I couldn't take advantage of using a block, so I've changed it to receive instrument as an argument.

elias19r · 2023-08-13T21:08:05Z

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

@@ -11,41 +11,50 @@ module Aggregation
        # Contains the implementation of the Sum aggregation
        # https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#sum-aggregation
        class Sum
-          def initialize(aggregation_temporality: :delta)
+          attr_reader :aggregation_temporality, :monotonic


Added attr_reader :monotonic. It is not being used in the class. I think it will be used later when we collect a MetricStream, but I can't quite see the big picture yet

elias19r · 2023-08-13T21:08:56Z

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

-            :counter
-          end
-
+        class Counter < OpenTelemetry::Metrics::Instrument::Counter


Made SDK Instrument classes inherit from API Instrument classes

elias19r · 2023-08-13T21:16:21Z

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

-          def default_aggregation
-            OpenTelemetry::SDK::Metrics::Aggregation::Sum.new
-          end


I've moved the default aggregations definition to Meter because it uses them to create MetricStream there. Thinking again, leaving definitions in each instrument class may look better but it would have to be a public method.

elias19r · 2023-08-13T21:20:11Z

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

-              snapshot = @metric_streams.map { |ms| ms.collect(@epoch_start_time, @epoch_end_time) }
+              metric_data = @metric_streams.map do |metric_stream|
+                metric_stream.collect(@epoch_start_time, @epoch_end_time)
+              end
              @epoch_start_time = @epoch_end_time
-              snapshot
+
+              metric_data


Just renamed snapshot to metric_data for a hint of the data type being returned

elias19r · 2023-08-13T21:26:34Z

sdk/lib/opentelemetry/sdk/instrumentation_scope.rb

+    InstrumentationScope = Struct.new(
+      :name,
+      :version,
+      :schema_url,
+      :attributes
+    )


TODO: Look up docs for why I added schema_url and attributes to InstrumentationScope 🤔

elias19r added 20 commits June 30, 2023 16:11

Move attr_reader from SDK to API in MeterProvider

d6c26fd

Call initialize from API in SDK MeterProvider and set Resource to def…

0876408

…ault See https://opentelemetry.io/docs/specs/otel/resource/sdk/

Move private_constant :Key from SDK to API in MeterProvider

1dd9e6f

Update InstrumentationScope to include schema_url and attributes

20ead90

See https://opentelemetry.io/docs/specs/otel/metrics/sdk/#meter-creation

Add @instrumentation_scope to Meter in API

ca7d03b

Update Key, the Meter identifier, to include schema_url

1555a62

See https://opentelemetry.io/docs/specs/otel/metrics/api/#get-a-meter > Meters are identified by `name`, `version`, and `schema_url` fields.

Update MeterProvider#meter in SDK

f5d3f03

Update ProxyMeterProvider to match API

159341d

Update ProxyMeter to match API

c33b137

Make Proxy Instruments inherit from API Instruments

f25ce0b

Require ProxyInstrument files

f21c303

Adjust SDK Meter to use register_instrument

30854b3

Add method #build_key_for_meter

508e19f

Remove SDK's SynchronousInstrument class

5db4749

Add #kind method to instruments

d8d929b

Not passing meter_provider down to meter and instruments

f7041b4

Move default_aggregation to MeterProvider

d442a9d

Make SDK metric Instruments inherit from API Instruments

94bcd68

Remove initialize methods from SDK async instruments

7590c1d

And instrument_kind methods

Remove require for SDK synchronous_instrument

838c516

elias19r changed the title ~~Adjust existing Metrics SDK to match changes to API~~ Adjust existing Metrics SDK code to match changes to API Jul 7, 2023

elias19r added 9 commits July 7, 2023 14:56

Add comment with agregation link to docs

a216f51

Keep InstrumentationScope in SDK

06b3c01

Add mutex for @DeleGate

a2ebb48

Trying to understando MetricData

f438baf

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

8c21fed

…ust-existing-metrics-sdk-code-to-match-api

Add more details to SDK Meter test

722035b

Add more details to MeterProvider test

acf5815

And I need to make the last test case work again. I thought we could avoid making Meter hold a reference to MeterProvider but that's no possible due to add_metric_reader

Make Meter hold a reference to MeterProvider

34fa220

and Instruments hold a reference to Meter

Build and create metric stream in Meter whenever a sync instrument is…

cef1294

… created

elias19r added 4 commits August 13, 2023 17:35

Add empty LastValue aggregation class

a76cb3e

Define default aggregation for instruments

e0b50ef

Make add_metric_stream methods similiar

19addcc

Also, - Remove .dup that I _think_ is not needed - Rename snapshot variable to metric_data

Make Aggregation::Sum test examples more verbose

63b6015

elias19r commented Aug 13, 2023

View reviewed changes

elias19r mentioned this pull request Aug 14, 2023

feat: Rework Meter and MeterProvider API open-telemetry/opentelemetry-ruby#1442

Closed

elias19r closed this Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust existing Metrics SDK code to match changes to API #1

Adjust existing Metrics SDK code to match changes to API #1

elias19r commented Jul 7, 2023 •

edited

Loading

elias19r Aug 13, 2023

elias19r Aug 13, 2023

elias19r Aug 13, 2023

elias19r Aug 13, 2023

elias19r Aug 13, 2023

elias19r Aug 13, 2023

elias19r Aug 13, 2023

elias19r Aug 13, 2023

elias19r Aug 13, 2023

Adjust existing Metrics SDK code to match changes to API #1

Adjust existing Metrics SDK code to match changes to API #1

Conversation

elias19r commented Jul 7, 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

elias19r commented Jul 7, 2023 •

edited

Loading