Repository navigation
Fix labelled heartbeat metrics reporting 0 with a buffered meter - #1593
Mahnoor-Zaffar wants to merge 4 commits into
Conversation
Sushisource
left a comment
There was a problem hiding this comment.
Overall I think this makes sense but we shouldn't need to be pulling in any updates from api_upstream in this PR
|
Just want to follow up how is this PR? Looks like it is pretty close to merge |
7d95a31 to
548887d
Compare
Sushisource
left a comment
There was a problem hiding this comment.
I went to update this to remove the bad rebase, but, on second thought looking at it again I don't think this is the right approach. If the issue was that the GIL had to be taken to read the BufferAttributes, that should be solved in the Python-specific implementation of BufferAttributes - not here in Core where it will apply to every language, including ones that don't have this issue.
Seems like this fix belongs in either the Python/Rust bridge code.
|
Figured out a way to simplify this that I liked. Will merge after I get another reviewer. |
What & Why
When a worker is configured with a buffered metric meter (e.g. the Python SDK's
MetricBuffer), every labelled worker-heartbeat metric — all slot counts and all poller counts — is reported to the server as0, even though the worker is demonstrably serving work. Unlabelled heartbeat metrics (total_processed_tasks, etc.) are unaffected.Root cause: Worker heartbeats do not query slot suppliers/pollers directly. They read
AtomicU64s that the metrics pipeline writes as a side effect viagauge_with_in_memory→HeartbeatMetricType::WithLabel. The label lookup inlabel_value_from_attributesonly handledPrometheus,OTel, andNoOp;MetricAttributes::Bufferfell through toNone, so the atomic was never written and kept its#[derive(Default)]value of0.The buffered handle is an opaque
LazyRef<Arc<dyn CustomMetricAttributes>>(a Python-heldPyDictin the Python bridge), which core cannot read without taking a GIL — exactly what the buffered design avoids. So core retains theVec<MetricKeyValue>it already has in hand at attribute-creation time instead.Changes
temporalio-common:MetricAttributes::Buffernow also carries a core-sidekvs: Arc<Vec<MetricKeyValue>>snapshot;label_value_from_attributesresolves labels from it.temporalio-sdk-core:MetricsCallBuffer::new_attributes/extend_attributescapture the kvs (merging last-wins on extend);BufferInstrument::senddestructures the new shape. TheMetricEventprotocol is unchanged, so language bridges compile untouched.HeartbeatMetricTypeandMetricsContext/in_memory_meterlevels, plus abufferedbacking added to the worker-heartbeat integration test.Unreleased.Validation
cargo test -p temporalio-common --features core-telemetry-bridge --lib in_memory_attributes_provide_label_values— passcargo test -p temporalio-sdk-core --lib telemetry::metrics— pass (incl. new regression; confirmed it fails without the fix)cargo +nightly fmt --checkclean.Fixes the Python SDK issue reported in temporalio/sdk-python#1817.