census-instrumentation / opencensus-python

A stats collection and distributed tracing framework
Apache License 2.0
666 stars 248 forks source link

Adds str fallback to envelope serialization #1196

Closed ddeschepper closed 1 year ago

ddeschepper commented 1 year ago

When serialization fails (e.g. when a non-JSON-serializable value is added to the properties) the entire batch that envelope belongs to does not get sent. Falling back to to str as a means of serialization when JSON serialization fails seems like a sane default that greatly reduces the risk of missed telemetry.

ddeschepper commented 1 year ago

resolves #1197

lzchen commented 1 year ago

@ddeschepper

I'm curious if you ran into a case in which the envelope was not serializable?

ddeschepper commented 1 year ago

@ddeschepper

I'm curious if you ran into a case in which the envelope was not serializable?

@lzchen I did: App Insights supports adding an additional "property bag", which is called customDimensions and which can be filled by adding a key "custom_dimensions" to the dictionary that can be provided to the "extra" argument, e.g:

logger.log(
    level,
    message,
    extra={
      'custom_dimensions'=my_additional_info
    }
)

If any of the values of the dict my_additional_info is not JSON-serializable, serializing the envelope fails. this happens for example with things as trivial as a datetime. The workaround of course, is to only add properties that are either JSON-serializable, or serialize them to string manually, but I think this minor addition makes life a lot easier, while also preventing lost telemetry.

lzchen commented 1 year ago

@ddeschepper Great! Thanks for the contribution! Could you add a CHANGELOG entry?

ddeschepper commented 1 year ago

@lzchen I've added the CHANGELOG entry.