Skip to content

Fix labelled heartbeat metrics reporting 0 with a buffered meter - #1593

Open
Mahnoor-Zaffar wants to merge 1 commit into
temporalio:mainfrom
Mahnoor-Zaffar:bmcqueen/heartbeat-metricbuffer-labels
Open

Mahnoor-Zaffar wants to merge 1 commit into
temporalio:mainfrom
Mahnoor-Zaffar:bmcqueen/heartbeat-metricbuffer-labels

Conversation

@Mahnoor-Zaffar

Copy link
Copy Markdown

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 as 0, 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 via gauge_with_in_memoryHeartbeatMetricType::WithLabel. The label lookup in label_value_from_attributes only handled Prometheus, OTel, and NoOp; MetricAttributes::Buffer fell through to None, so the atomic was never written and kept its #[derive(Default)] value of 0.

The buffered handle is an opaque LazyRef<Arc<dyn CustomMetricAttributes>> (a Python-held PyDict in the Python bridge), which core cannot read without taking a GIL — exactly what the buffered design avoids. So core retains the Vec<MetricKeyValue> it already has in hand at attribute-creation time instead.

Changes

  • temporalio-common: MetricAttributes::Buffer now also carries a core-side kvs: Arc<Vec<MetricKeyValue>> snapshot; label_value_from_attributes resolves labels from it.
  • temporalio-sdk-core: MetricsCallBuffer::new_attributes/extend_attributes capture the kvs (merging last-wins on extend); BufferInstrument::send destructures the new shape. The MetricEvent protocol is unchanged, so language bridges compile untouched.
  • Tests: unit regression covering buffered label resolution at both the HeartbeatMetricType and MetricsContext/in_memory_meter levels, plus a buffered backing added to the worker-heartbeat integration test.
  • Changelog entry under Unreleased.

Validation

  • cargo test -p temporalio-common --features core-telemetry-bridge --lib in_memory_attributes_provide_label_values — pass
  • cargo test -p temporalio-sdk-core --lib telemetry::metrics — pass (incl. new regression; confirmed it fails without the fix)
  • Integration test target compiles; clippy clean for these changes; cargo +nightly fmt --check clean.

Fixes the Python SDK issue reported in temporalio/sdk-python#1817.

@Mahnoor-Zaffar
Mahnoor-Zaffar requested a review from a team as a code owner September 5, 2026 13:42
@CLAassistant

CLAassistant commented Sep 5, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@Sushisource Sushisource left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall I think this makes sense but we shouldn't need to be pulling in any updates from api_upstream in this PR

@bobingm

bobingm commented Sep 14, 2026

Copy link
Copy Markdown

Just want to follow up how is this PR? Looks like it is pretty close to merge

@Sushisource
Sushisource force-pushed the bmcqueen/heartbeat-metricbuffer-labels branch from 7d95a31 to 548887d Compare September 14, 2026 22:00
@Sushisource
Sushisource enabled auto-merge (squash) September 14, 2026 22:01

@Sushisource Sushisource left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

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.

4 participants