Skip to content

Conversation

@jan-auer
Copy link
Member

@jan-auer jan-auer commented Dec 18, 2018

Updates the logentry interface and removes some deprecated behavior.

Changes to the interface

  • Make logentry.message optional, and require one of {message, formatted}
  • Make logentry.formatted the canonical key, remove logentry.message if they are the same
  • Only support python-style interpolation (%s and %(key))
  • Trim everything at the end, after interpolating

Changes to normalization

  • logentry, sentry.interfaces.Message and message are all aliases in ascending priority
  • message is no longer written to logentry.formatted

Note:

  • There is still no difference between missing and empty params.
  • Empty logentry.message used to generate an error while empty message did not. This behavior is preserved.

#sync-getsentry

@jan-auer jan-auer self-assigned this Dec 18, 2018
@jan-auer
Copy link
Member Author

jan-auer commented Dec 18, 2018

Test results from 50.000 events:

  • 3 (~0.006%) different groups due to non-string messages (e.g. "message": 105)
  • 500 (~1%) events with differences in the logentry interface (in decending priority):
    • instances where people sent both logentry and message. Often times message contained a better formatting than what we interpolate. I guess this is an instance where people have to update the clients or tweak their messages.
    • message with {} instead of %s interpolation from certain clients using sentry-java
    • messages with non-primitive format arguments. We want to drop support for this, so this is fine.
    • JSON objects as message. This is also removed on purpose.
    • slight differences in trimming, since the formatted message wasn't trimmed before.

I also found one event that uses %r in the format string, which we will not support in Rust going forward. This will create another diff (but no different groups) when switching over.

@jan-auer
Copy link
Member Author

All the percy diffs are caused by 103985a

@mitsuhiko
Copy link
Contributor

@jan-auer i would propose we implement %r as a json dump. This won't affect grouping anyways and that makes more sense than anything else. I would still support providing the markers for extended formatting (like whitespace control etc.) but just ignore them on rendering.

{} i'm very much in favor of dropping still, going to look tomorrow more in detail at the diffs. I think it's okay though since it won't affect grouping and the info still shows up in the UI. we might however consider reaching out to people we think this might affect.

About this:

JSON objects as message. This is also removed on purpose.

I'm still okay to stop supporting this but at the same time I'm not sure if it wouldn't make sense to just stringify it out. It is still very possible to generate such events by accident in many SDKs and it might just be user hostile in some situations to reject this.

@jan-auer jan-auer requested a review from mitsuhiko December 20, 2018 10:41
@jan-auer jan-auer force-pushed the ref/cleanup-logentry branch from 103985a to 0d7c9e0 Compare January 7, 2019 08:24
@jan-auer
Copy link
Member Author

jan-auer commented Jan 7, 2019

Updated. +1 on JSON-formatting %r, although we don't have to implement that in python anymore.
I also re-introduced formatting of {}, as this is used as default format in a number of languages and might make it easier for certain SDKs to send correct format strings.

@jan-auer jan-auer merged commit 5ef5e51 into master Jan 7, 2019
@jan-auer jan-auer deleted the ref/cleanup-logentry branch January 7, 2019 16:34
jan-auer added a commit that referenced this pull request Jan 9, 2019
Fixes and improves the rendering of message (logentry) params which was broken
in #11078. Also, `logentry.formatted` is now guaranteed to always be present and
there is no more fallback to `logentry.message`.
tkaemming added a commit that referenced this pull request Feb 20, 2019
This was probably broken with GH-11078 but it's hard to tell for sure
because the logger is disabled that reports feature extraction failure.
This only really presents itself as a failure in development when
bootstrapping an environment with `bin/load-mocks`.
tkaemming added a commit that referenced this pull request Feb 20, 2019
This was probably broken with GH-11078 but it's hard to tell for sure
because the logger is disabled that reports feature extraction failure.
This only really presents itself as a failure in development when
bootstrapping an environment with `bin/load-mocks`.
@github-actions github-actions bot locked and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants