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

Merged
merged 17 commits into from
Dec 10, 2024
Merged

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will create an issue to track it (e.g. something like integrate metrics_sdk to sdk?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just seeing this! Yes, I think something like that would work and add a link to this code for reference. This is a good example for a similar TODO for logs: #1739

@kaylareopelle kaylareopelle merged commit bb51595 into open-telemetry:main Dec 10, 2024
66 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Implement Synchronous Gauge Instrument
3 participants