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.

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

Adds str fallback to envelope serialization #1196

Conversation

ddeschepper
Copy link
Contributor

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.

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
Copy link
Contributor Author

resolves #1197

@lzchen
Copy link
Contributor

lzchen commented Mar 14, 2023

@ddeschepper

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

@ddeschepper
Copy link
Contributor Author

ddeschepper commented Mar 16, 2023

@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
Copy link
Contributor

lzchen commented Mar 16, 2023

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

@ddeschepper
Copy link
Contributor Author

@lzchen I've added the CHANGELOG entry.

@lzchen lzchen merged commit 3a2d8df into census-instrumentation:master Mar 16, 2023
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.

None yet

2 participants