Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
|
| # Not keyed by id(): the buffer frees a metric or attribute set once | ||
| # Core drops it, and a later one can then get the same id(). |
There was a problem hiding this comment.
Let's remove the comment here about the previous implementation.
| assert instrument.gauge_state is not None | ||
| with instrument.gauge_state.lock: | ||
| instrument.gauge_state.last_values[id(update.attributes)] = ( | ||
| instrument.gauge_state.last_values[frozenset(attributes.items())] = ( |
There was a problem hiding this comment.
I think we should avoid computing this frozenset each update. But I do think we should use the content based key.
Let's restore the attribute cache and precompute this in a bounded cache. Maybe something like this?
id(attrs) -> (
attrs, # keep ref to avoid id reuse
dict(attrs), # passed to otel
frozenset(dict(attrs)), # used as gauge values cache key
)There was a problem hiding this comment.
Done in a468e66, for attributes and for instruments.
Each entry holds its object, so the id can't be reused while it is cached; at 1000 entries a cache starts over. An attribute entry holds the dict passed to OTel and the gauge key, built on the first gauge update (counters and histograms don't use it).
One change from the sketch: the key is frozenset(attrs.items()). frozenset(dict(attrs)) would hold only the keys, so {"model": "a"} and {"model": "b"} would share one gauge value.
Timing, previous commit → now (main in brackets; median of 15, debug bridge build):
- 20 long-lived attribute sets, 102k updates: 317 → 277 ms (278).
- a new attribute set per call, as Core's per-activity metrics make, 68k updates: 242 → 261 ms (235). The cache can't hit there, so each update pays the dict and an insert.
Most of the gain is the instrument cache in front of (name, kind). With it alone, without the attribute cache, the two cases measured 289 and 221 ms, so I can drop the attribute cache if you'd rather.
Each entry keeps its object alive, so the id can't be reused while the entry is cached, and both caches are bounded: a full cache starts over. An attribute entry also keeps the gauge key once a gauge update has built it. Instruments are still shared by (name, kind) behind the metric cache.
What was changed
MetricsExporter(temporalio/contrib/opentelemetry/_metrics_exporter.py) caches converted attributes byid(attributes)and OTel instruments byid(metric), but keeps neither object alive. When Core drops an attribute set or a metric, its Python object is freed, the next one can get the same address, and the exporter then hands back the old labels or the old instrument.This change makes each cache entry hold its object, so the id can't be reused while the entry is cached, and bounds both caches at 1000 entries (a full cache starts over). An attribute entry also holds the gauge key, the attribute items as a
frozenset, built on the first gauge update, so a gauge's last values are keyed by contents without rebuilding the key on every update. Behind the metric cache, instruments are keyed by(name, kind), so instruments created per call share one OTel instrument. That also fixes gauges created per call: OTel ignores the callback of a secondcreate_observable_gaugewith the same name, so on main such a gauge exported only the first call's value.Why?
The buffer creates new objects all the time. The bridge makes a new Python dict per attribute set and a new
BufferedMetricper instrument (temporalio/bridge/src/metric.rs), and Core makes a fresh attribute set on everynew_attributes/extend_attributescall, for example to recordactivity_execution_latencyfor each activity completion. User code does the same withactivity.metric_meter().with_additional_attributes({...})or by creating instruments per call. The runtime docs promise the same object across updates for the same attribute set, which holds only while that object is alive.What it looks like: three calls creating custom metrics per call (gpt-a with 100 tokens and 900 ms, llama-b with 7 and 40, qwen-c with 55 and 300). One run on main exported
llm_tokens{model="gpt-a"} = 195and allm_latency_ms{model="gpt-a"}histogram with count 3 and sum 1207: llama-b's latency went into the token counter and its token count into the histogram. With this change: 100, 7 and 55, and one latency sample per model.Cost (debug bridge build, median of 15): with 20 long-lived attribute sets, applying 102k buffered updates takes 277 ms (main: 278). With a new attribute set per call, as Core's per-activity metrics make, 68k updates take 261 ms (main: 235), because the id caches can't hit there.
Checklist
Closes: no issue (small bug fix).
How was this tested:
test_metrics_exporter_instruments_created_per_callcreates a counter, a histogram and a gauge under{"model": ...}in three calls, drains, and checks every value. On main it fails (for example{'model-a': 162} != {'model-a': 100}; 30 of 30 runs with pytest-flakefinder); with the change it passes (30 of 30).test_metrics_exporter_caches_are_boundedcaps both caches at 2 entries, sends five attribute sets twice, and checks that the caches stay within 2 entries and that every value is right after eviction.poe lintpasses.tests/contrib/opentelemetry: 88 passed.mac_core_metrics_check.py, two activity types, six runs each):activity_execution_latencysamples byactivity_typewere{'None': 1, 'beta': 6, 'alpha': 5}with the original exporter and{'alpha': 6, 'beta': 6}with this change.Any docs updates needed? No. Changelog fragment:
changelog/fixed/mislabeled-marmoset.md.