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

Support float and duration metrics #508

Merged
merged 2 commits into from
Apr 18, 2024
Merged

Conversation

cretz
Copy link
Member

@cretz cretz commented Apr 12, 2024

What was changed

  • Added MetricMeter.create_histogram_float, MetricMeter.create_histogram_timedelta, and MetricMeter.create_gauge_float
  • Accept duration_format on MetricBuffer create. It defaults to milliseconds, but can be set to seconds
  • Built abstractions for the existing and new metrics to inherit common things
  • Added tests

Checklist

  1. Closes [Feature Request] Support metric option for using seconds (and other recent core metric changes) #493

@cretz cretz requested a review from a team as a code owner April 12, 2024 22:20
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

Makes sense. I think you could push timedelta down even further though.

"""Initialize histogram."""
self._ref = meter._ref.new_histogram_duration(name, description, unit)

def record(self, value_ms: int, attrs: MetricAttributes) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Why not pass through timedelta here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not the most trivial to create these types of objects on the Rust side and there's a cost to creating them on the Rust side (have to capture GIL, etc). I considered converting back to timedelta closer to the user (and I do it in .NET), but I would have to decide whether to create it on the Rust side or Python side (today each getter of a Python buffered metric value is actually backed by Rust values which is neat). The Rust side is a pain, and the Python side is expensive and unnecessary since the user can do it themselves.

But I have left open this possibility. I have an enum of MetricBufferDurationFormat and if we want to add a third TIMEDELTA enum we can (like we did in .NET).

Copy link
Member

Choose a reason for hiding this comment

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

K, that's reasonable. Being able to record them as timedeltas is the main thing that matters and that works.

@cretz cretz merged commit 50c2033 into temporalio:main Apr 18, 2024
12 checks passed
@cretz cretz deleted the metric-durations branch April 18, 2024 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Support metric option for using seconds (and other recent core metric changes)
2 participants