Skip to content

feat(tracing)!: send observation metadata as one JSON attribute - #1944

Closed
niklassemmler wants to merge 9 commits into
mainfrom
lfe-17153-python-sdk-next-major-send-observation-metadata-as-one-json
Closed

niklassemmler wants to merge 9 commits into
mainfrom
lfe-17153-python-sdk-next-major-send-observation-metadata-as-one-json

Conversation

@niklassemmler

@niklassemmler niklassemmler commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Linear: LFE-17153
JS counterpart: langfuse/langfuse-js#994 (LFE-17151)

Summary

Observation metadata is now written as one langfuse.observation.metadata JSON attribute instead of one langfuse.observation.metadata.<key> attribute per key. This is the Python side of langfuse-js#994. Both SDKs should ship it in their next major, otherwise they store nested metadata differently (primer issue [5]). Background: appendix of the metadata workshop primer. The server already parses the bare attribute (extractMetadata in OtelIngestionProcessor), so no server change is needed.

  • attributes.py: new MAX_OBSERVATION_METADATA_KEYS = 128, serialize_observation_metadata() and merge_observation_metadata(). Each value is serialized on its own (compact JSON via EventSerializer), so one value that fails becomes "<failed to serialize>" and the other keys survive.
  • span.py: the serialized metadata is kept per OTel span (WeakKeyDictionary, under a lock), so update(), update_current_span() and update_current_generation() still merge by top-level key and write the full merged blob each time. Keys set to None keep their earlier values. The merge state is keyed by the OTel span, not the wrapper, because the update_current_* helpers create a new wrapper for every call.
  • OpenAI and LangChain integrations: metadata over the limit is dropped with a warning instead of raising (same as @langfuse/vercel-ai-sdk in the JS PR). Raising would fail the user's OpenAI call, because start_observation runs outside the wrapper's try. In LangChain it would lose the observation.

⚠️ Breaking changes

  1. Wire format. Observation metadata is sent as a single langfuse.observation.metadata JSON attribute. Nothing writes langfuse.observation.metadata.<key> anymore. Custom span processors or exporters that read the per-key attributes must parse the blob instead.
  2. Stored shape of nested metadata. The server deep-flattens the blob, so {"database": {"host": "localhost"}} is now stored as key database.host instead of key database holding a JSON string. Saved filters and dashboard widgets on the top-level key won't match new data. Arrays are still stored as one JSON-string value. Strings and ints used to be sent as native attribute values. They're now JSON values inside the blob, and the server stores them the same way as before.
  3. Limit of 128 metadata keys. More than 128 top-level keys on one observation (counted after merging updates, ignoring None values) now raises ValueError from start_observation, start_as_current_observation, update() and update_current_span() / update_current_generation(). Before, OpenTelemetry's 128-attribute span limit dropped the extra keys silently. When it raises, the span is left unchanged (metadata, other attributes in the same call, and the name).
  4. mask_otel_spans sees one metadata attribute. This differs from JS. In Python, the SDK-level mask function already ran on observation metadata (it masks the metadata dict before serialization), so nothing changes there. Only the export-stage mask_otel_spans hook now receives a single langfuse.observation.metadata JSON string instead of per-key attributes.
  5. Length limits now apply to the whole metadata object. With OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT set, the limit now cuts the whole metadata JSON. The cut JSON fails to parse on the server, so all metadata on that span is dropped (before, only the one long value was cut).

Not changed: trace metadata from propagate_attributes stays per-key (langfuse.trace.metadata.<key>).

Parity with langfuse-js#994

Same in both SDKs: attribute name and shape; compact JSON; merge by top-level key across updates and update_current_* / updateActiveObservation; None/null keeps the earlier value; empty metadata writes no attribute; non-dict metadata is written as is (strings raw, other values as JSON) and resets the merge state; the limit counts merged keys after dropping None; on error nothing changes; the error message is the same (Observation metadata has N keys, which exceeds the maximum of 128.); integrations drop the metadata with a warning; trace metadata stays per-key.

Differences that remain, all deliberate or outside this change:

  • Value encoding comes from each SDK's serializer. Python uses EventSerializer (shared with input/output), JS uses JSON.stringify. The server re-parses the blob and stores leaves with JSON.stringify, so 1.0 vs 1, é vs é and spacing all end up identical in storage. Values whose encoding actually differs: NaN/Infinity (Python "NaN", JS null), circular references (Python nests up to EventSerializer's depth limit, JS "<failed to serialize>"), datetimes (Python ...00Z, JS ...00.000Z). These already differed for input/output and aren't changed here.
  • Merge state is a snapshot in Python. Python keeps the serialized values, so a caller mutating a metadata dict after passing it doesn't change earlier metadata. JS keeps the object reference (the issue Greptile raised on perf: move serialization to background threads #994). JS should probably snapshot too.
  • Mask semantics, see breaking change 4.

Notes for reviewers

  • How to review: the source change is ~300 lines (attributes.py, span.py, client.py, openai.py, CallbackHandler.py). Most of the diff is tests. In tests/unit/test_otel.py the old per-key metadata tests are replaced, not edited, so read the new TestMetadataHandling tests on their own instead of diffing them.
  • When the 128-key check raises in start_observation, the OTel span has already started and is never ended (it's dropped, not exported). Same as JS. With start_as_current_observation, OTel's context manager ends the span.
  • The integrations check the key count when the observation starts. Integration-added keys that push a later merge over the limit are still caught by each integration's existing exception handling.
  • Known trade-off: with OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT (or custom SpanLimits) set, OTel truncates the single metadata JSON string, the server can't parse it, and all metadata on that span is dropped. Before, only the one oversized key was cut. The limit is unset by default, input/output already have the same exposure as single JSON strings, and JS has the same design. Handling oversized JSON attributes is left for a follow-up covering both SDKs.
  • Sibling PR fix(tracing): serialize non-string propagated metadata as JSON #1945 fixes propagated trace metadata (which stays per-key here) being stored as Python reprs (['branch:agent']) instead of JSON.

Verification

  • uv run --frozen ruff check . ✅
  • uv run --frozen ruff format --check ✅
  • uv run --frozen mypy langfuse --no-error-summary ✅
  • uv run --frozen pytest -n auto --dist worksteal tests/unit --deselect tests/unit/test_prompt.py: 701 passed ✅. Covers the new TestMetadataHandling tests (wire format, merging, None handling, update_current_*, mutation, failure fallback, 128 limit incl. merged count and unchanged span, mask, threaded updates) and the new OpenAI/LangChain key-limit tests.
  • tests/unit/test_prompt.py: 18 errors, same on main (Langfuse client is not initialized, a local env issue unrelated to this change).
  • tests/e2e: not run, no Langfuse server available. Worth running against a server to confirm nested metadata comes back deep-flattened (test_media.py and test_decorators.py read nested metadata through the API).

🤖 Generated with Claude Code

RetriggerConfidence Score: 4/5

Fix the forked-worker hang and satisfy the import rule before merging.

Summary

Observation metadata now uses one JSON attribute. Updates merge serialized values by top-level key, enforce a 128-key limit, and share merge state across wrappers of the same span.

  • OpenAI and LangChain drop oversized initial metadata with a warning.
  • The new shared lock needs a child-process reset to prevent hangs after a fork.
  • New test imports must move to the module’s import section.

Acknowledgments from niklassemmler: the wire format, nested storage shape, export-stage masking, and length truncation changes are deliberate breaking changes. The unended span after a start-time limit error is known. Later integration overflow relies on existing exception handling. Trace metadata coercion is deferred to a separate PR.

Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Observation start or update] --> B[Process media and apply mask]
  B --> C[Serialize each metadata value]
  C --> D[Acquire shared metadata lock]
  D --> E[Merge keys for this OTel span]
  E --> F{More than 128 keys?}
  F -->|Yes| G[Raise before writing attributes]
  F -->|No| H[Save merge state and write one JSON attribute]
Loading

Reviews (1) · Last reviewed commit: "feat(tracing)!: send observation metadat..." · Reviewed by Greptile

Write observation metadata as a single `langfuse.observation.metadata` JSON
attribute instead of one `langfuse.observation.metadata.<key>` attribute per
key, matching langfuse-js#994.

- Keep serialized metadata per OTel span so update() and the
  update_current_* helpers merge by top-level key. None values keep earlier
  values.
- Serialize values one by one; a failing value becomes
  "<failed to serialize>".
- Raise ValueError when merged metadata has more than 128 top-level keys,
  leaving the span unchanged. The OpenAI and LangChain integrations drop the
  metadata with a warning instead.
- Propagated trace metadata stays per key.

BREAKING CHANGE: observation metadata wire format changed, nested metadata
is stored as dotted keys, and more than 128 metadata keys raises.

Linear: LFE-17153

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown

@claude review

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

No review was started: this request came from a bot account. Manual reviews can only be requested by someone with write access to this repository. Ask a maintainer to comment @claude review, or have your automation post the comment from a user account with write access.

Tip: disable this comment in your organization's Code Review settings.

Comment thread langfuse/_client/span.py
Comment thread tests/unit/test_otel.py Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I reviewed this PR; beyond the inline findings above, I also checked the new global _span_metadata_lock in langfuse/_client/span.py for contention risk under concurrent tracing — the critical section only does a dict lookup, the (already-serialized) merge, and span.set_attributes, so it's short and doesn't appear to be a correctness or significant performance issue.

Extended reasoning...

The diff reworks observation-metadata serialization from per-key OTel attributes to a single merged JSON attribute, touching attributes.py, span.py, client.py, the LangChain/OpenAI integrations, and tests; it adds a global lock plus a WeakKeyDictionary to merge metadata across update calls and a new 128-key cap that raises ValueError. No auth/crypto/permission surface is touched, but the change affects error handling paths in widely-used integrations, and three confirmed correctness findings (silent loss of generation updates/experiment metadata/LangChain root metadata on key-limit overflow) are already queued as inline comments, so this is not an approve. I independently checked the new single global lock for contention risk given concurrent tracing is common; the critical section is small (dict lookup, merge, set_attributes) so it did not look like an additional bug worth flagging.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🔴 langfuse/openai.py — For OpenAI streaming calls, a metadata-key overflow during stream finalize now silently drops the whole generation update, not just metadata. _finalize_stream_response (langfuse/openai.py:1067-1096) calls _create_langfuse_update with metadata extracted fresh from the response, without drop_metadata_over_key_limit. If merging it with the generation's existing metadata pushes the key count over MAX_OBSERVATION_METADATA_KEYS (128), generation.update() now raises ValueError, which the bare except Exception: pass at line 1093 swallows -- discarding output, usage_details, cost_details and model along with metadata, where the base branch's flatten-based approach never raised and still recorded output/usage. Fix: guard metadata passed into generation.update() in _create_langfuse_update / _finalize_stream_response with drop_metadata_over_key_limit (as already done for the creation-time metadata at lines 1307 and 1396), so an over-limit merge degrades metadata only, not the whole update.

    Why this was flagged

    Trigger: an OpenAI streaming call (chat, Responses API, or legacy) whose generation already has metadata close to the 128-key cap (set via the create() metadata kwarg, capped only at start_observation time via drop_metadata_over_key_limit at langfuse/openai.py:1307/1396) and whose streamed response itself contributes additional top-level metadata keys via _extract_streamed_response_api_response/_extract_streamed_openai_response (langfuse/openai.py:774-794, 797+). Merging in _set_attributes_with_merged_metadata (langfuse/_client/span.py) raises ValueError when total keys exceed 128. That error propagates through generation.update() (langfuse/_client/span.py:787-789) and _create_langfuse_update (langfuse/openai.py:714-726) to the bare except Exception: pass in _finalize_stream_response (langfuse/openai.py:1093-1094), so output, usage, cost and model are never written even though generation.end() still runs in the finally block. On the base branch the old _flatten_and_serialize_metadata never raised, so output/usage were always recorded regardless of metadata size.

    Verification: normal (narrow but valid trigger; new data-loss path not on base). The streaming finalize path does not guard streamed metadata with drop_metadata_over_key_limit, and the whole generation update is swallowed on overflow.

    langfuse/openai.py:1075-1096: _finalize_stream_response wraps _create_langfuse_update(..., metadata=metadata) in try/except Exception: pass, with generation.end() in finally.

  • 🔴 langfuse/_client/client.py — run_experiment now silently fails whole dataset items whose combined metadata exceeds 128 keys, instead of just truncating the extra metadata as before. final_observation_metadata (client.py:2973-2995) is passed to start_as_current_observation(metadata=...) at client.py:3013-3018, which now raises ValueError via merge_observation_metadata once merged keys exceed 128. Unlike openai.py/CallbackHandler.py, this call site was not updated to call drop_metadata_over_key_limit first. Fix: call drop_metadata_over_key_limit(final_observation_metadata) before passing it to start_as_current_observation, so experiment items with many metadata keys degrade instead of failing the item.

    Why this was flagged

    Trigger: a dataset item whose item.metadata (or experiment_metadata) has close to 128 top-level keys, reached via Langfuse.run_experiment -> _run_experiment_async -> _process_experiment_item (client.py:2937). final_observation_metadata adds experiment_name, experiment_run_name, and for dataset items dataset_id/dataset_item_id, pushing the merged key count over 128. start_as_current_observation(metadata=final_observation_metadata) at client.py:3013-3018 propagates into LangfuseObservationWrapper.init -> _set_attributes_with_merged_metadata (span.py:78) -> merge_observation_metadata (attributes.py:232), which raises ValueError. The except Exception at client.py:3089 catches it, marks the item-run span ERROR, and re-raises; _run_experiment_async's asyncio.gather(..., return_exceptions=True) (client.py:2857) then drops this item from valid_results (client.py:2862-2865), logging "Item %s failed". On the base branch, OTel's 128-attribute span cap silently dropped excess metadata keys and the item still completed; now the whole item is excluded from experiment results.

    Verification: Mechanism is real and reachable. run_experiment builds final_observation_metadata (client.py:2973-2995) and passes it to start_as_current_observation at client.py:3013-3018, which raises ValueError once merged keys exceed MAX_OBSERVATION_METADATA_KEYS=128 (attributes.py:262-267). The raise is caught at client.py:3089 and swallowed by asyncio.gather(return_exceptions=True) at client.py:2857, silently dropping the entire item result.

Comment thread langfuse/langchain/CallbackHandler.py Outdated
Address review findings on the observation metadata change:

- Reset the span metadata lock in forked children, so a lock held by a
  parent thread at fork time can't deadlock the child.
- OpenAI stream finalize: when the streamed metadata pushes the merged
  metadata over the key limit, drop the metadata and keep output, usage and
  cost instead of losing the whole update. Add
  ObservationMetadataKeyLimitError (a ValueError) so this is caught exactly.
- run_experiment: drop over-limit item metadata with a warning instead of
  failing the item.
- Move test imports to module level.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@niklassemmler

Copy link
Copy Markdown
Contributor Author

@claude review

🤖 Written by Claude (an AI agent) on behalf of Niklas

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

Root runs add a synthetic `is_langchain_root` key, which pushed metadata with
exactly 128 user keys over the limit and dropped all of it. Check the user
metadata against the limit first, and only add the marker if it fits.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@niklassemmler

Copy link
Copy Markdown
Contributor Author

@claude review

🤖 Written by Claude (an AI agent) on behalf of Niklas

@niklassemmler

Copy link
Copy Markdown
Contributor Author

@greptileai review

🤖 Written by Claude (an AI agent) on behalf of Niklas

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Beyond the inline findings, I checked two other candidates and ruled them out: the post-fork handler only resets _span_metadata_lock, not the WeakKeyDictionary itself, but fork's copy-on-write memory preserves the dict's existing entries intact in the child, so no metadata goes missing there; and start_as_current_observation raising ValueError when merged metadata exceeds 128 keys does skip the whole with block, but that matches the PR's documented breaking-change behavior ("span is left unchanged... when it raises") rather than being an unintended bug.

Extended reasoning...

This change reworks observation metadata to a single JSON OTel attribute with merge-by-key semantics, a module-level lock plus fork-safety hook in span.py, and proactive/catch-based drop-on-limit handling in the OpenAI and LangChain integrations; it touches concurrency (shared lock across all attribute writes) and data integrity (metadata serialization/merging) but no auth or injection surface. The two confirmed findings already queued as inline comments (silent metadata loss under OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT, and a process-wide lock now serializing all span attribute writes) are sufficient on their own to warrant human attention, so this note only adds the two extra candidates examined and ruled out this run.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Comment thread langfuse/_client/attributes.py
Comment thread langfuse/_client/span.py Outdated
…lock

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@niklassemmler

Copy link
Copy Markdown
Contributor Author

@claude review

🤖 Written by Claude (an AI agent) on behalf of Niklas

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread langfuse/_client/span.py
@niklassemmler

Copy link
Copy Markdown
Contributor Author

Re the out-of-diff findings in the review summary:

  • OpenAI stream finalize losing the whole update on metadata overflow: fixed in 96d9018. _create_langfuse_update catches ObservationMetadataKeyLimitError, drops only the metadata and retries, so output, usage and cost stay. Covered by test_openai_stream_keeps_output_when_metadata_exceeds_key_limit.
  • run_experiment failing items on over-limit metadata: fixed in the same commit, the metadata is dropped with a warning.

All inline threads have replies.

🤖 Written by Claude (an AI agent) on behalf of Niklas

Every metadata value went through json.dumps(cls=EventSerializer), which
builds an encoder per call (~1us vs ~0.1us). For str, bool, finite float
and JS-safe int the output is identical, so encode those directly.

update() with 10 string keys: 22us -> 13us per call (main: 11us).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@niklassemmler

Copy link
Copy Markdown
Contributor Author

@claude review

🤖 Written by Claude (an AI agent) on behalf of Niklas

Each update rebuilt the metadata JSON by re-encoding every earlier key
with json.dumps, which dominated the cost on wide spans. Store the
encoded `"key":value` members instead and only encode new ones.

1-key update on a span with 128 keys: 28.6us -> 9.4us (main: 6.6us).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@niklassemmler

Copy link
Copy Markdown
Contributor Author

@claude review

🤖 Written by Claude (an AI agent) on behalf of Niklas

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Beyond the inline finding, this run also checked two other candidates and ruled them out: the new scalar fast-path in _serialize_metadata_value (attributes.py) skips the try/except, but json.dumps never raises on plain str/bool/JS-safe int/finite float, so no new failure path is introduced; and mask_otel_spans now seeing one merged langfuse.observation.metadata JSON attribute instead of per-key attributes is a deliberate, documented breaking change in the PR description rather than a silent regression.

Extended reasoning...

This run re-read the diff against the four prior rounds of feedback: the langchain root-run 128-key issue (CallbackHandler.py) was actually fixed in commit 3076401 (checks user metadata against the limit before adding the synthetic key), and the fork-safety/degrade-gracefully fixes landed in 96d9018. The attribute-length-truncation and lock-scope points from earlier rounds remain open design trade-offs the author explicitly accepted in the PR description rather than code fixes. This run's own new candidates (scalar-serialization fast path, mask_otel_spans shape change) were checked against the code and ruled out as described above.

This pull request has been reviewed before and this review found new issues. Where they share a root cause, one fix may close them together.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🔴 langfuse/_client/span.py — A custom mask function that errors (or legitimately redacts the whole metadata object) on one update() call now erases ALL previously recorded observation metadata for that span, not just the current call's data. _mask_attribute (span.py:627-651) falls back to the string "" on exception; this non-dict value hits merge_observation_metadata (attributes.py:260-264), which treats any non-dict metadata as a full reset (returns merged=None), so _set_attributes_with_merged_metadata (span.py:106-129) overwrites the single langfuse.observation.metadata attribute with just that string, discarding keys written by earlier successful update()/start_observation() calls on the same span. …

    Why this was flagged

    …Before this diff each update wrote distinct langfuse.observation.metadata. attributes, so a later non-dict/failed-mask value only added one stray attribute and never erased earlier per-key metadata. Fix: make the mask-failure fallback (and any other non-dict value the SDK itself produces, as opposed to metadata the caller explicitly passed as non-dict) preserve previously merged metadata, e.g. by wrapping the fallback string back into a dict value for the unmasked keys, or special-casing exception fallback separately from the documented…

    Trigger: a Langfuse client configured with mask= (span.py reads it via self._langfuse_client._mask) whose function raises on some later update() call's metadata, e.g. due to a bug or an unexpected value shape, while an earlier update()/start_observation() on the same span already wrote dict metadata. At span.py:644-651 the exception handler returns the literal string "" instead of propagating or preserving structure.

    Verification: nit. The mechanism is real and reachable. When the custom mask function raises, _mask_attribute (span.py:644-651) returns the literal "<fully masked due to failed mask function>". For metadata this non-dict string flows to serialize_observation_metadata, which for a non-dict returns _serialize_metadata_value(value, top_level=True) → the raw string (attributes.py:228-229, 312-313). merge_observation_metadata then hits if isinstance(serialized, str): return serialized, None (attributes.py:263-264), so merged=None.

When a custom mask function raised on dict metadata, the fallback string
replaced the span's whole metadata, including keys from earlier updates.
For metadata, mask each key of the failed call with the fallback instead,
so it merges like any other update.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@niklassemmler

Copy link
Copy Markdown
Contributor Author

@claude review

🤖 Written by Claude (an AI agent) on behalf of Niklas

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread langfuse/openai.py Outdated
Comment on lines 550 to +555
root_metadata = observation_metadata.copy() if observation_metadata else {}
root_metadata["is_langchain_root"] = True

# Only add the marker if it fits, so root runs keep the full key budget
key_count = sum(1 for value in root_metadata.values() if value is not None)
if key_count < MAX_OBSERVATION_METADATA_KEYS:
root_metadata["is_langchain_root"] = True

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 (optional) Root LangChain runs with user metadata just under the cap now crash on a later legitimate update, which never happened on base. When a root run's metadata has 127 keys, CallbackHandler.py:552-555 silently adds an internal is_langchain_root key, bringing the span to exactly 128 keys. Any later call that adds even one more key on that span (e.g. update_current_span()/update_current_generation(), which client.py never wraps in try/except) then hits merge_observation_metadata's 129>128 check (attributes.py:273-278) and raises ObservationMetadataKeyLimitError straight into user code. Fix: don't count internal bookkeeping keys like is_langchain_root against the user's 128-key budget.

Why this was flagged

Trigger: a LangChain root run whose combined tags+metadata has 127 keys, just under MAX_OBSERVATION_METADATA_KEYS=128. CallbackHandler.py:553-555 computes key_count=127<128 and adds root_metadata['is_langchain_root']=True, making the span's stored metadata exactly 128 keys. Any subsequent call on that span adding one more key (e.g. update_current_span()/update_current_generation()) makes merge_observation_metadata (attributes.py:268-278) compute merged=129>128 and raise ObservationMetadataKeyLimitError. client.py's update_current_span/update_current_generation have no try/except around generation.update()/span.update(), so this ValueError propagates into the caller's code and crashes it, even though the user's own metadata never exceeded 128 keys. On base there was no 128-key hard cap, so this crash is new.

Verification: The scenario is real and reachable but low severity, a narrow specialization of intended behavior. CallbackHandler.py:553-555 adds is_langchain_root to a root run when key_count (127) < 128, producing 128 keys stored in span.py's _span_metadata cache. A later update_current_span() call (client.py:1561-1569) hits merge_observation_metadata (attributes.py), computing len(merged)=129>128, raising ObservationMetadataKeyLimitError uncaught. Base had no key cap, so this did not crash there.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Leaving this as is. is_langchain_root gets stored as a real metadata key, so it counts. Leaving it out of the count would mean adding an exception for integration keys to the core merge, and the count would then differ from langfuse-js#994, which counts all merged non-null keys. The case is narrow: exactly 127 user keys on the root run, then a later update that adds a new key to the root span itself. update_current_* inside a chain usually hits a child run. With 128 user keys the marker is already skipped (3076401).

🤖 Written by Claude (an AI agent) on behalf of Niklas

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Still open from earlier reviews (2):

  • Unresolved: 2 minor or pre-existing.

Comment thread langfuse/_client/span.py Outdated
… fails

The mask-failure fallback replaced every key of the metadata dict, including
keys set to None, which should keep their earlier values.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ey limit

Retrying the whole update after a metadata key limit error masked the output
and enqueued its media twice. Update metadata separately instead, so only the
metadata is dropped.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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.

1 participant