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: add basic implementation of asynchronous metrics #1610

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

xuan-cao-swi
Copy link
Contributor

Description

I'd like to make contribution on basic implementation on asynchronous metrics.

Related: #1386, Asynchronous Up Down Counter card, Asynchronous Gauge.
Spec: asynchronous-instrument-api

WIP:

  1. when multiple callbacks exist in single instrument, they may all impose (accumulative) modification on data points, which need further consideration of use case.

Copy link
Contributor

github-actions bot commented Apr 4, 2024

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

@github-actions github-actions bot added the stale label Apr 4, 2024
@kaylareopelle kaylareopelle added keep and removed stale labels Apr 4, 2024
end
end

def create_callback(callbacks)

Choose a reason for hiding this comment

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

I think create_callback is a bit of left over from some experimentation. We should probably remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks!

class ObservableCounter < OpenTelemetry::Metrics::Instrument::ObservableCounter
attr_reader :name, :unit, :description
# {ObservableCounter} is the SDK implementation of {OpenTelemetry::SDK::Metrics::Instrument::AsynchronousInstrument}.
# Asynchronous Counter is an asynchronous Instrument which reports non-additive, monotonically increasing value(s) when the instrument is being observed.

Choose a reason for hiding this comment

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

This comment is a bit confusing. I think it should read

Asynchronous Counter is an asynchronous Instrument which reports monotonically increasing value(s) when the instrument is being observed.

See the spec
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#asynchronous-counter

@kaylareopelle kaylareopelle linked an issue Oct 16, 2024 that may be closed by this pull request
@dan-corneanu
Copy link

Any chance one of the code owners could take a look at this PR?

@zvkemp
Copy link

zvkemp commented Jan 8, 2025

I think there are a few other things to consider here; top of my list would be the ability to return multiple values from a callback, meaning the callback should also be able to return arbitrary attributes (perhaps in addition to the attributes declared when the instrument is created). An example might be reporting variable-length statistics about the current state of the system, like activity-per-cpu or RSS for each pid:

callback = lambda do |state|
  state.observe(2000, { 'pid' => 12, 'process_name' => 'puma' })
  state.observe(2500, { 'pid' => 14, 'process_name' => 'sidekiq' })
end

(In this example, the state object is some data container that exists for this callback invocation; the next operation would be to loop over each observed result and call update with the value and callback attributes, probably merged with the instrument's attributes).

The spec has a few other examples similar to this.

@xuan-cao-swi
Copy link
Contributor Author

Hi @zvkemp, thank you for your feedback. Initially, I considered only a single value. However, we can address this by modifying the operation here to check the type of value returned from the user's code block.

@zvkemp
Copy link

zvkemp commented Jan 9, 2025

As far as I can tell, the expected modes of the callback are:

  1. it returns a single value
  2. it returns a value along with some attributes (e.g. [1, { 'cpu' => '1' }])
  3. it returns multiple values
  4. it returns multiple values, each with their own attributes

Since the actual API of the callback isn't defined by the spec, it should be valid to specify that in order to return multiple values, a helper object should be used. Otherwise, for simple cases it should be ok to just return a value.

This was the thing I prototyped (before noticing this PR):

class `
  attr_reader :data
  def initialize
    @data = []
  end

  def observe(value, attributes = nil)
    @data << [value, attributes]
    self
  end

  def self.enum_for(value)
    if value.is_a?(self)
      value.data.to_enum
    else
      # assume we have a single value
      [[value, nil]].to_enum
    end
  end
end

invoke_callback would be:

          def invoke_callback(timeout, attributes)
            @mutex.synchronize do
              Timeout.timeout(timeout || 30) do
                @callback.each do |cb|
                   observations = CallbackObservations.new
                  CallbackObservations.enum_for(cb.call(observations)).each do |value, attributes|
                    @aggregation.update(value, attributes || {}, @data_points)
                  end
                end
              end
            end
          end

... and then callbacks could be any of the following:

-> (obs) do
  obs.observe(1)
end

-> (obs) do
  obs.observe(1, cpu: 1)
end

-> () { 1 }

This is sort of maximally flexible without needing to do nested type-checking (but doesn't yet include any validation on the return values of the callbacks — they are assumed to be the observations object or a single value). A safer version would be to enforce that all observations are recorded via CallbackObservations#observe, and ignore the return values from the procs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

Asynchronous Counter
5 participants