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. |
|
|
| 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.
There was a problem hiding this comment.
Thanks for exploring this option. After seeing the added complexity, I agree that we should drop the attributes cache and keep the main change of the instrument cache key.
There was a problem hiding this comment.
Dropped the attribute cache in fa6cb01. Attributes are converted on each update again, and the gauge key is frozenset(attributes.items()). The id() cache stays only in front of the (name, kind) instrument map, and the bounded test now checks just that cache.
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.
Attributes are converted on each update again, and a gauge's last values are keyed by frozenset(attributes.items()). The id() cache stays only in front of the (name, kind) instrument map, and the bounded test now checks only that cache.
| class _IdCache(Generic[_K, _V]): | ||
| # Keyed by id() for speed. Each entry holds its object, so the id can't be | ||
| # reused by another object while the entry is cached. A full cache starts | ||
| # over, which bounds what it keeps alive. | ||
| def __init__(self, max_size: int) -> None: | ||
| self._max_size = max_size | ||
| self._entries: dict[int, tuple[_K, _V]] = {} | ||
|
|
||
| def get(self, obj: _K) -> _V | None: | ||
| entry = self._entries.get(id(obj)) | ||
| return None if entry is None else entry[1] | ||
|
|
||
| def put(self, obj: _K, value: _V) -> None: | ||
| if len(self._entries) >= self._max_size: | ||
| self._entries.clear() | ||
| self._entries[id(obj)] = (obj, value) | ||
|
|
||
| def __len__(self) -> int: | ||
| return len(self._entries) | ||
|
|
||
|
|
||
| _ID_CACHE_SIZE = 1000 | ||
|
|
There was a problem hiding this comment.
Let's simplify by removing this and go back to a dict with the key fix. After that, looks gtm!
Thanks for your patience and contribution!
There was a problem hiding this comment.
Removed _IdCache in 0bcaa05: instruments are back in a plain dict keyed by (name, kind), and the bounded-cache test is gone. tests/contrib/opentelemetry: 87 passed; the per-call test still fails 30 of 30 on main's exporter. My earlier ping crossed with your review, sorry for the noise.
Instruments are looked up by (name, kind) in a plain dict again. The bounded-cache test goes with the cache.
|
@VegetarianOrc could you take another look when you have time? fa6cb01 drops the attribute cache as you suggested (details in the thread above). |
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 drops both caches: attributes are converted on each update, and a gauge's last values are keyed by the attribute items as a
frozensetinstead of byid(attributes). Instruments are kept in a plain dict 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 (release bridge, Python 3.14, median of 15, two runs): with 20 long-lived attribute sets, applying 102k buffered updates takes 267-301 ms (main: 238-241), since each update converts its attributes again. With a new attribute set per call, as Core's per-activity metrics make, 68k updates take 215-225 ms (main: 235-246).
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).poe lintpasses.tests/contrib/opentelemetry: 87 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.