Skip to content
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.

Sign up for GitHub

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

Serialize pydantic events before creating OTEL spans #64

Merged
merged 4 commits into from
Aug 6, 2024
Merged

Serialize pydantic events before creating OTEL spans #64

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
df5b145
otel: derive event name early for tracing
ebr Jul 19, 2024
0d7c2c1
otel: serialize pydantic models when creating spans
ebr Jul 19, 2024
a0fb7fb
otel: serialize pydantic models when injecting trace parents
ebr Jul 19, 2024
a2f9dff
fix(otel): ensure backwards compatibility with pydantic-v1 and with n…
ebr Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion fastapi_events/dispatcher.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,8 @@ class UserCreated(pydantic.BaseModel):
# Handle dispatch without event_name specified
if not event_name and isinstance(event_name_or_model, (str, Enum)):
event_name = event_name_or_model
elif hasattr(event_name_or_model, "__event_name__"):
event_name = event_name_or_model.__event_name__

with create_span_for_dispatch_fn(event_name=event_name):
if HAS_PYDANTIC:
Expand All @@ -290,7 +292,7 @@ class UserCreated(pydantic.BaseModel):
)

# OTEL
if payload and isinstance(payload, dict):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why this type check was needed here, and whether injecting traceparent is needed for every payload or only some.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@melvinkcx I don't see the removal of this typing check breaking anything, but let me know please if i'm missing something.

Also, in this PR we're serializing the Pydantic payload for OTEL inside both create_span_for_handle_fn() and inject_traceparent() . Do you think there's a performance penalty here that is worth spending the time to optimize?

Thank you for this great library :)

Copy link
Owner

Choose a reason for hiding this comment

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

IIRC, the isinstance(payload, dict) was added just in case people are using a custom class instance as payload. In this case, we should avoid injecting traceparent, since it most likely wont work.

Copy link
Owner

Choose a reason for hiding this comment

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

For the performance penalty, I see your point. I don't have any numbers on hand as for how much performance penalty it introduces, but I wouldn't mind us fixing it

if payload:
logger.debug("Injecting traceparent to event payload...")
inject_traceparent(payload=payload)

Expand Down
7 changes: 7 additions & 0 deletions fastapi_events/otel/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from fastapi_events.otel import HAS_OTEL_INSTALLED, propagate, trace
from fastapi_events.otel.attributes import SpanAttributes
from fastapi_events.utils import strtobool
from pydantic import BaseModel

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -39,6 +40,9 @@ def create_span_for_handle_fn(

links, context = [], None

if isinstance(payload, BaseModel):
Copy link
Owner

@melvinkcx melvinkcx Jul 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, thanks for your contribution :)

Even though the package is called fastapi_events, I know there are some users using it without FastAPI and Pydantic. So, importing pydantic will break those use cases.

Instead of importing BaseModel from pydantic, I suggest using duck typing to maximise backward compatibility.

Also, IIRC, the model_dump method was introduced in Pydantic v2. It replaced the dict() method in v1.

I'd suggest something like:

if hasattr(payload, "model_dump"):  # handle Pydantic v2
    payload = payload.model_dump()
elif hasattr(payload, "dict"):    # handles Pydantic v1
    payload = payload.dict()

In fact, we have similar logic handling both pydantic versions and the absence of pydantic in the dispatcher just fyi:

Once you update the changes, I'll be sure to have it merged asap. Thanks again for your contribution :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the changes as you recommended. Duck typing seemed less verbose in this case than duplicating the HAS_PYDANTIC... logic from the dispatcher, but i'd be fine with either approach.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks, it works for me

payload = payload.model_dump()

# Extract span from remote context
remote_ctx = propagate.extract(payload)
if use_span_linking:
Expand Down Expand Up @@ -85,6 +89,9 @@ def inject_traceparent(payload: Dict):
logger.debug("Unable to inject traceparent. OTEL is not installed.")
return

if isinstance(payload, BaseModel):
payload = payload.model_dump()

if not isinstance(payload, dict):
logger.debug("Unable to inject traceparent. Payload is not a dict")
return
Expand Down