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 synchronous gauge #1718

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

Conversation

xuan-cao-swi
Copy link
Contributor

Description:

Add simple gauge metrics that only record last value of recording.

Closes #1704

TODO:

  1. Replace the default aggregation with last_value once this pull request is merged.
  2. Update the test case to better represent how a gauge should behave differently from a counter.
  3. Update the metrics exporter to include the gauge type.

@xuan-cao-swi xuan-cao-swi changed the title feat: add gauge feat: add synchronous gauge Sep 4, 2024
Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Thanks for opening this, Xuan! Even more reason to get #1604 merged.

Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

I'm going to mark this as "request changes" because I don't think we should merge this in until it uses LastValue as the default aggregation.

Copy link
Contributor

👋 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

@KieranP
Copy link

KieranP commented Nov 2, 2024

I'd like to make my interest in this known. I'm in the process of moving all my production applications from Datadog to self-hosted Signoz, which rely on OTel providers. We currently have multiple areas where we report current state of things (such as background queue size and latency) via gauge metrics. Without this Gauge metric, I'm not able to complete the migration. So would be very keen to see this completed in a future release.

@xuan-cao-swi
Copy link
Contributor Author

Updated the aggregation to last value. Updated the test case.
Will work on otlp exporter once this pr get review and merge

_(last_snapshot[0].data_points[0].value).must_equal(1)
end

it 'gauge should count 1 for last recording' do
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this needs a different title for the assertion? It seems like the goal here is to make sure two different gauges can be evaluated individually?


private

# TODO: replace the default aggregation to LastValue
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# TODO: replace the default aggregation to LastValue

Comment on lines +28 to +30
# TODO: When the metrics SDK stabilizes and is merged into the main SDK,
# we can leverage the SDK Internal validation classes to enforce this:
# https://github.com/open-telemetry/opentelemetry-ruby/blob/6bec625ef49004f364457c26263df421526b60d6/sdk/lib/opentelemetry/sdk/internal.rb#L47
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we turn this TODO into an issue, or reference it on an existing issue?

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

Successfully merging this pull request may close these issues.

Implement Synchronous Gauge Instrument
3 participants