-
Notifications
You must be signed in to change notification settings - Fork 244
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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.
Co-authored-by: Kayla Reopelle <[email protected]>
Co-authored-by: Kayla Reopelle <[email protected]>
Co-authored-by: Kayla Reopelle <[email protected]>
Co-authored-by: Kayla Reopelle <[email protected]>
Co-authored-by: Kayla Reopelle <[email protected]>
Co-authored-by: Kayla Reopelle <[email protected]>
Co-authored-by: Kayla Reopelle <[email protected]>
👋 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 |
…uby into issues-1704
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. |
Updated the aggregation to last value. Updated the test case. |
_(last_snapshot[0].data_points[0].value).must_equal(1) | ||
end | ||
|
||
it 'gauge should count 1 for last recording' do |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# TODO: replace the default aggregation to LastValue |
# 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 |
There was a problem hiding this comment.
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?
Description:
Add simple gauge metrics that only record last value of recording.
Closes #1704
TODO: