Skip to content

Fix serializer negative infinity#1684

Open
aryxnn wants to merge 2 commits into
langfuse:mainfrom
aryxnn:fix-serializer-negative-infinity
Open

Fix serializer negative infinity#1684
aryxnn wants to merge 2 commits into
langfuse:mainfrom
aryxnn:fix-serializer-negative-infinity

Conversation

@aryxnn
Copy link
Copy Markdown

@aryxnn aryxnn commented Jun 1, 2026

What does this PR do?

PR title must follow Conventional Commits, for example feat: add dataset scoring helper or fix(openai): preserve trace context.

Fixes #

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Refactor
  • Documentation update
  • Tooling, CI, or repo maintenance

Verification

List the main commands you ran:

poetry run pytest tests/unit/test_serializer.py

Checklist

  • I self-reviewed the diff using code_review.md.
  • I added or updated tests for behavior changes.
  • I updated docs, examples, or .env.template if needed.
  • I did not hand-edit generated files; if generated files changed, I used the upstream regeneration path.
  • I did not commit secrets or credentials.

Greptile Summary

This PR fixes two bugs: negative infinity floats were always serialised as "Infinity" instead of "-Infinity", and propagate_attributes rejected non-string metadata values instead of coercing them.

  • serializer.py: one-line fix to return "-Infinity" when math.isinf(obj) is true and obj < 0.
  • propagation.py: widens the metadata parameter type to Dict[str, Any] and adds str(value) coercion for non-string values; None values are now silently skipped rather than propagated.
  • Tests: new test_float_special_values covers all three float special values; a new propagation test verifies int/float/bool coercion; test-data fixture classes renamed from Test* to Mock* to avoid unintentional pytest collection.

Confidence Score: 4/5

Safe to merge — the serializer fix is a minimal one-liner with a clear test, and the propagation coercion change is well-covered by the new test.

Both fixes are small and targeted. The only gap is the docstring for propagate_attributes which still tells callers that non-string metadata values are dropped, when they are now silently coerced via str(). A caller relying on the old drop-with-warning behaviour could be surprised.

langfuse/_client/propagation.py — the docstring for the metadata parameter needs to reflect the new coercion behaviour.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[EventSerializer.default called with float] --> B{math.isnan?}
    B -- yes --> C[return 'NaN']
    B -- no --> D{math.isinf?}
    D -- no --> E[normal float path]
    D -- yes --> F{obj < 0?}
    F -- yes --> G[return '-Infinity']
    F -- no --> H[return 'Infinity']

    I[propagate_attributes metadata dict] --> J{value is None?}
    J -- yes --> K[skip / continue]
    J -- no --> L{isinstance str?}
    L -- yes --> M[use as-is]
    L -- no --> N[str coerce via str value]
    M --> O[_validate_string_value]
    N --> O
    O -- valid --> P[add to validated_metadata]
    O -- invalid --> Q[drop with warning]
Loading
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
langfuse/_client/propagation.py:126-130
The docstring still says non-string metadata values "will be dropped with warning" (`AVOID: ... non-string values (will be dropped with warning)`), but the new code now coerces them to strings via `str(value)`. A caller who reads the docstring before passing `int`/`float`/`bool` values will incorrectly expect them to be silently dropped.

```suggestion
        metadata: Additional key-value metadata to propagate to all spans.
            - Keys must be US-ASCII strings
            - String values must be ≤200 characters; non-string values are coerced via ``str()``
            - ``None`` values are silently skipped
            - Use for dimensions like internal correlating identifiers
            - AVOID: large payloads, sensitive data
```

### Issue 2 of 2
tests/unit/test_serializer.py:307
The file is missing a trailing newline. Most linters and editors (and git) expect text files to end with a newline character.

```suggestion
    assert serializer.encode(float("nan")) == '"NaN"'
```

Reviews (1): Last reviewed commit: "fix: serialize negative infinity as -Inf..." | Re-trigger Greptile

Aryan Srivastava added 2 commits June 1, 2026 03:06
Allows propagate_attributes to accept and automatically convert non-string metadata values (int, float, bool) to strings instead of silently dropping them. This resolves tracing compatibility issues with LangGraph integrations.
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Aryan Srivastava seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

2 participants