Repository navigation
fix(tracing): mask dict metadata per key when the mask function fails - #1956
Conversation
1e1e202 to
47d263b
Compare
|
@claude review |
There was a problem hiding this comment.
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.
|
@claude review 🤖 Written by Claude (an AI agent) on behalf of Niklas |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds new mask-resolution and OTel context/baggage propagation logic around sensitive metadata masking, a human look would still be worthwhile.
What was reviewed: _get_current_mask()'s client resolution (public-key context vs. single-instance fallback under LangfuseResourceManager._lock) — the multi-client-silently-skips-masking scenario matches the documented single-client/@ observe usage pattern, not a regression; the per-value masking in _propagate_attributes before serialization/200-char truncation — runs once per value, keys preserved, exception scoped to that value; the experiment/dataset metadata masking gap — intentional scope limit per the PR description, not a new regression.
Extended reasoning...
This PR (two commits since the diff base) adds per-key mask fallback for observation metadata in span.py and extends masking to propagate_attributes() in propagation.py, including a new lock-protected client/mask resolution helper and per-value masking before OTel context/baggage writes — code that directly affects whether sensitive data is exposed in traces. The second commit (mask propagation) is new since my last review of this PR and is non-trivial, so it merits a fresh pass. No new bugs were found this run; two candidate concerns about multi-client mask resolution and experiment-metadata masking scope were examined and ruled out as intentional/documented behavior rather than regressions. Given the security-sensitive surface and added complexity, a human review is still worthwhile even though no defects were found.
|
@claude review 🤖 Written by Claude (an AI agent) on behalf of Niklas |
When a custom mask function raised on dict metadata, the fallback string was written to the bare langfuse.observation.metadata attribute. The server parses that attribute as a JSON object, so the fallback was lost, and nothing showed which keys had been masked. For dict metadata, write the fallback under each key of the failed call instead, so it merges with keys from earlier updates like any other update. Non-dict metadata, input and output keep the plain-string fallback. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
propagate_attributes now applies the client's mask to each propagated trace metadata value (keys are kept), on the raw value before serialization and the 200 character check. If the mask raises, only that value becomes the fallback string. The masked values are stored in the context and baggage, so the current span, child spans, and outgoing baggage all carry them. Matches langfuse-js #1003. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…adata The mask for propagated trace metadata comes from the client for the public key in the execution context, otherwise the only initialized client. With several clients and no public key in context, or with no client yet, propagated metadata is not masked. Document this and the workarounds, and pin the behavior with a test. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
d852fda to
37a31a2
Compare
Propagated metadata is passed explicitly by the developer. Masking it adds client-resolution ambiguity and baggage/length-limit ordering complexity, and diverges from the JS SDK. Co-authored-by: Hassieb Pakzad <hassiebp@users.noreply.github.com>
|
Trimmed at Hassieb's request: masking of propagated trace metadata is removed (68f16e4), and only the per-key mask-failure fallback for dict observation metadata remains. Propagated metadata is passed explicitly by the developer, so it's already in their control; masking it added client-resolution ambiguity and baggage/length-limit ordering complexity, and diverged from the JS SDK. |
…metadata Co-authored-by: Hassieb Pakzad <hassiebp@users.noreply.github.com>
Problem
When the custom
maskfunction raises,_mask_attributereturns the string"<fully masked due to failed mask function>". For dict metadata that string goes through the non-dict branch of_flatten_and_serialize_metadataand is written to the barelangfuse.observation.metadataattribute.Repro: start an observation with
metadata={"a": 1}, thenupdate(metadata={"secret": "pw", "b": 2})with a mask that raises on the second call. The span ends with:The server parses the bare attribute as a JSON object, so the fallback is lost (or counted as a parse failure). Nothing shows which keys were masked.
Separately,
masknever ran on propagated trace metadata (propagate_attributes(metadata=...), written aslangfuse.trace.metadata.<key>), so those values were exported unmasked.Fix
_process_media_and_apply_masknow passesfieldto_mask_attribute. If the mask fails andfield == "metadata"and the data is a dict, the fallback is{key: fallback for key in data}. Each key of the failed call is written aslangfuse.observation.metadata.<key>, keys from earlier updates are kept, and no bare attribute is written. Non-dict metadata, input and output still get the plain-string fallback._process_media_and_apply_maskis the only caller of_mask_attribute, and every metadata path goes through it (start,update, and_set_processed_span_attributes), so they all get the same behavior.The fallback string is now a shared constant,
MASK_FALLBACK_VALUEinlangfuse/_client/constants.py.Masking propagated trace metadata
propagate_attributesnow applies the client'smaskto each propagated trace metadata value. The mask gets only the value, never the key, so keys are kept. If the mask raises, only that value becomes"<fully masked due to failed mask function>". Compared with langfuse-js #1003: keys are kept and a failing mask only replaces that value in both SDKs. The differences come from Python masking inpropagate_attributeswhile JS masks at export; see the examples below._propagate_attributes, before values are written to the context. The masked values are what gets stored in the OTel context and in baggage, so the current span, every child span (copied by the span processor on start), and outgoing baggage to other services all carry them. The mask runs once per value, not once per span.get_client()would pick: the one for the public key in the execution context, otherwise the only initialized client. With no client, or with several clients and no public key in context, no mask is applied. Without a configured mask nothing changes.propagate_attributesruns, propagated metadata is not masked, even if a client has a mask. Workaround: passlangfuse_public_keyvia@observe, or use a single client.mask_otel_spansalso sees thelangfuse.trace.metadata.*attributes at export, but not outgoing baggage. This is documented on themaskparameter, thepropagate_attributesmetadataargument, and_get_current_mask(), and pinned by a test.metadatais masked. Experiment metadata set internally by the SDK is not.maskdocstring onLangfuseand themetadatadocs onpropagate_attributesnow describe this.Behavior change by example
Mask fails on dict observation metadata
langfuse.observation.metadata.a11langfuse.observation.metadata"<fully masked due to failed mask function>"langfuse.observation.metadata.secret"<fully masked due to failed mask function>"langfuse.observation.metadata.b"<fully masked due to failed mask function>"The mask still gets the whole dict in one call, as before. So when it fails, every key from that call is masked. Non-dict metadata, input and output still get the plain string.
Propagated trace metadata is now masked, per value
langfuse.trace.metadata.api_key"secret-key""***-key"langfuse.trace.metadata.auth'{"token":"secret"}''{"token":"***"}'The mask gets the raw Python value (the dict for
auth), never the key, and runs once per value, not once per span. In JS (#1003) the mask gets the serialized string and runs again for every span.A throwing mask only replaces the value it failed on
A mask that raises for
"secret", withmetadata={"api_key": "secret", "env": "prod"}, exportsapi_keyas"<fully masked due to failed mask function>"andenvas"prod".Baggage is masked too
With
as_baggage=True, thelangfuse_metadata_api_keybaggage entry sent to downstream services carries the masked value. In JS, baggage is sent unmasked.The 200 character limit is checked after masking
With a mask that returns
data[:10],metadata={"long": "a" * 300}used to be dropped and is now exported as"aaaaaaaaaa". A mask result over 200 characters is dropped. JS checks the limit before masking. Masking first is deliberate: the limit then applies to the value that actually gets exported, and a value that is too long is never trimmed into a short version of something the mask would have redacted.This is one of three small PRs replacing the abandoned single-JSON-blob approach in #1944 (LFE-17153).
Verification
tests/unit/test_otel.py::TestMetadataHandling: mask fails on update (earlier keys kept, each failed key masked, no bare attribute); mask fails at start with dict metadata; non-dict metadata, input and output keep the string fallback. The first two failed before the fix.tests/unit/test_propagate_attributes.py::TestPropagateAttributesMask: masked values on the current span and on a child span, keys kept, the mask called once per raw value; masked values in baggage; a throwing mask only replaces that value; the 200 character limit applies to the masked value; no mask means no change; with two clients the mask applies only when the public key is in context. The first four failed before the change.uv run --frozen ruff check .: passesuv run --frozen ruff format --check .: flags onlytests/unit/test_media.py, which this PR does not touch (already the case on main)uv run --frozen mypy langfuse --no-error-summary: passesuv run --frozen pytest -n auto --dist worksteal tests/unit: 773 passed, 2 skipped, 18 errors. All 18 errors are intests/unit/test_prompt.py("Langfuse client is not initialized"). They also happen locally on main and are unrelated to this change.🤖 Generated with Claude Code