Serialize pydantic events before creating OTEL spans

[ebr]

OpenTelemetry propagate.extract() method is unable to handle Pydantic models, so they must be serialized before creating spans. Looks like the issue was introduced with #57 and only appears when Pydantic model schema dump is turned off.

In this PR we also determine the event name before creating the span. Otherwise all dispatch spans are named Event None dispatched - now the actual event name is set instead of None.

Fixes #63

[ebr]

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

[ebr]

@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 :)

[melvinkcx]

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.

[melvinkcx]

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

[melvinkcx]

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:

• fastapi-events/fastapi_events/dispatcher.py

  Line 22 in f44705b

  IS_PYDANTIC_V1 = False

• fastapi-events/fastapi_events/dispatcher.py

  Line 156 in f44705b

  if IS_PYDANTIC_V1:

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

[ebr]

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.

[melvinkcx]

Thanks, it works for me