Skip to content

🐛 Fix verbose console formatting for enum types (#1073) #1096

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

sbhrwlr
Copy link

@sbhrwlr sbhrwlr commented May 29, 2025

Fixes #1073
Change:
Explicitly handled Enums by returning logfire_json_dumps(value), which ensures consistent output across all Enum types.

How I Tested:
Manually tested with all Enum types (Enum, IntEnum, StrEnum, Flag, IntFlag). Please validate the output below for each Enum variant.
Screenshot 2025-05-29 at 12 08 59 PM

Copy link

codecov bot commented May 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@sbhrwlr sbhrwlr marked this pull request as draft May 29, 2025 05:29
@sbhrwlr sbhrwlr marked this pull request as ready for review May 29, 2025 06:39
@alexmojaki
Copy link
Contributor

Thanks! Please extend test_other_json_schema_types to cover enums.

Also note that this PR doesn't close the issue as decimals and datetimes are still incorrect.

@sbhrwlr sbhrwlr marked this pull request as draft May 29, 2025 11:57
@sbhrwlr sbhrwlr marked this pull request as ready for review May 29, 2025 17:30
@sbhrwlr
Copy link
Author

sbhrwlr commented May 29, 2025

Screenshot 2025-05-29 at 10 57 19 PM

@alexmojaki Changes include a fix, format updates in tests, and new test cases for different enum types. Let me know if anything needs attention.

@@ -978,8 +978,8 @@ class MyPydanticDataclass:
'code.lineno': 123,
'code.function': 'test_log_non_scalar_complex_args',
'a': 1,
'complex_list': '["a",1,{"x":"x","y":"2023-01-01T00:00:00"},{"t":10},{"p":20}]',
'complex_dict': '{"k1":"v1","model":{"x":"x","y":"2023-01-01T00:00:00"},"dataclass":{"t":10},"pydantic_dataclass":{"p":20}}',
'complex_list': '["a",1,{"x":"x","y":"datetime.datetime(2023, 1, 1, 0, 0)"},{"t":10},{"p":20}]',
Copy link
Contributor

Choose a reason for hiding this comment

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

This part can't change. The actual attributes should contain the kind of data you'd expect to find in JSON. Python reprs should only be in the console.

Copy link
Author

@sbhrwlr sbhrwlr May 29, 2025

Choose a reason for hiding this comment

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

You're right, this behavior stems from Pydantic's data serialization approach. Refer json_encoder.py method _pydantic_model_encoder

The Problem

When using the default model_dump(), Pydantic preserves Python objects in their native form:

from pydantic import BaseModel
from fractions import Fraction
from datetime import datetime

class MyModel(BaseModel):
   x: Fraction
   y: datetime

model = MyModel(x=Fraction(1, 3), y=datetime.now())

Serialization behaviour comparison

# Default mode - preserves Python objects
model.model_dump()
# Output: {'x': '1/3', 'y': datetime.datetime(2025, 5, 30, 0, 29, 36, 610411)}

# JSON mode - converts to JSON-serializable types
model.model_dump(mode="json")
# Output: {'x': '1/3', 'y': '2025-05-30T00:29:36.610411'}

The Challenge

Using model_dump(mode="json") would fix serialization consistency but breaks the test_recursive_objects test.
so should we use model_dump(mode="json") and change test_recursive_objects accordingly?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's not about pydantic models. We always want datetimes to be represented as ISO strings in the attributes, even if they're at the top level.

Copy link
Author

Choose a reason for hiding this comment

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

@alexmojaki This behavior should be consistent across similar cases. For instance, I tested using Fraction(3, 4) at the root level. Instead of getting '3/4' as the output, the value returned was Fraction(3, 4) — which is its repr().

Copy link
Contributor

Choose a reason for hiding this comment

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

It just so happens that Pydantic dumps fractions to strings (which TBH I find odd in Python mode) and that logfire doesn't have any handling for fractions. This isn't intentional. It would make sense for logfire to always produce 3/4 too.

@sbhrwlr sbhrwlr marked this pull request as draft May 30, 2025 13:00
@sbhrwlr sbhrwlr marked this pull request as ready for review May 30, 2025 14:40
@sbhrwlr
Copy link
Author

sbhrwlr commented May 30, 2025

@alexmojaki Tried these changes:

  • Introduced dedicated methods: _format_date, _format_datetime, _format_time, selected via x-python-datatype in the schema.
  • Added x-python-datatype: Added x-python-datatype in the schema for datetime, date, time & decimal
  • Updated formatter to parse string values using ISO format methods or use the object directly if already the correct type.
  • Updated tests to ensure all edge cases and input types are handled.
Screenshot 2025-05-30 at 8 13 13 PM

@alexmojaki
Copy link
Contributor

Please check that it still looks right in the logfire UI, the x-python-datatype might change things.

@sbhrwlr
Copy link
Author

sbhrwlr commented May 30, 2025

Sure, @alexmojaki Please check below screenshot for local console and logfire UI.

Test Code

logfire.info(
    'sample test new with datetime, date, time, and decimal',
    dt=datetime(2025, 5, 30, 12, 34, 56, 789000),
    da=date(2025, 5, 30),
    ti=time(12, 34, 56, 789000),
    dec=Decimal(1.0)
)

Output Comparison

Local Console Output

Local Console Screenshot

Logfire UI Output

Logfire UI Screenshot

@alexmojaki let me know if there are any concerns

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.

Fix verbose console formatting for some types
3 participants