Buffered metrics #391
Buffered metrics #391
Conversation
| metrics::MetricValue::String(v) => new_attrs.set_item(kv.key, v), | ||
| metrics::MetricValue::Int(v) => new_attrs.set_item(kv.key, v), | ||
| metrics::MetricValue::Float(v) => new_attrs.set_item(kv.key, v), | ||
| metrics::MetricValue::Bool(v) => new_attrs.set_item(kv.key, v), |
There was a problem hiding this comment.
You should be able to do this all with one branch like:
metrics::MetricValue::String(v) | metrics::MetricValue::Int(v) | metrics::MetricValue::Float(v) | metrics::MetricValue::Bool(v) => new_attrs.set_item(kv.key, v)
There was a problem hiding this comment.
I had a thing where I had the match just in the value but Rust was struggling to confirm they all impl'd ToPyObject w/out a manual dyn hint. I just figured this was the easiest, but will try to change to that if it works on next commit.
EDIT: Confirmed Rust is not smart enough to do proper inference of v here, you get something like:
294 | metrics::MetricValue::String(v) | metrics::MetricValue::Int(v) | metrics::MetricValue::Float(v) | metrics::MetricValue::Bool(v) => new_attrs.set_item(kv.key, v)
| - ^ expected `String`, found `i64`
| |
| first introduced with type `std::string::String` here
|
= note: in the same arm, a binding must have the same type in all alternatives
There was a problem hiding this comment.
Oh, yeah, they'd need to be turned into trait objects. Not worth it.
temporalio/bridge/src/runtime.rs
Outdated
| self.runtime | ||
| .metrics_call_buffer | ||
| .as_ref() | ||
| .unwrap() |
There was a problem hiding this comment.
There's enough unwrapping going on in this callstack it might make sense to just actually have this return a Result
There was a problem hiding this comment.
I considered that, but I decided to trust the caller. In theory the Python code will never call this unless it was setup with a buffer. So there isn't a user-facing failure here that should happen.
There was a problem hiding this comment.
Changed to expect to clarify but I never expect it to panic (or I definitely would return an error to be bubbled out through Python)
temporalio/runtime.py
Outdated
| and is drained regularly. See :py:class:`MetricBuffer` warning. | ||
|
|
||
| Args: | ||
| buffer_size: Size of the buffer. Set this to a large value. |
There was a problem hiding this comment.
Some guidance beyond large is useful here I think
There was a problem hiding this comment.
Suggestions? I didn't know of a good value so I intentionally didn't set a default. I don't know how frequently updates are emitted to add any more info.
There was a problem hiding this comment.
I added "A value in the tens of thousands or higher is plenty reasonable" but not sure what other guidance I can provide that I don't already with warnings at the class level and elsewhere explaining that frequent draining is required. Suggestions?
There was a problem hiding this comment.
I think that's good. 10k ish seems reasonable. It's hard to guess at.
What was changed
Support for user-retrieved metrics via a buffer
MetricBuffer,BufferedMetric, andBufferedMetricUpdatetotemporalio.runtimeChecklist