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

Fix default fingerprinting when using synthetic traces #1246

Closed

Conversation

jcruz2us
Copy link

@jcruz2us jcruz2us commented Mar 6, 2018

I think I've found a bug w/ fingerprinting and synthetic traces.

When using synthetic traces via the globalOption stracktrace: true, there is an attempt to maintain the legacy grouping behavior and fingerprint on msg, not stack trace.

Looks like when this was implemented, there was a few oversights. Here is the breaking code

  1. The default fingerprint is dropped and never gets to sentry
  2. The fingerprint value is of the wrong type

First, there is an attempt to create a default fingerprint from the msg param passed to captureMessage. The fingerprint property is added to the options variable but the values from options are not merged back to data after the fingerprint property is set. The end-result is that fingerprint ends up getting dropped and it never makes it to sentry.

The other issue w/ the existing code is that fingerprint is set to a string value. Sentry does not like raw strings and it will ignore the fingerprint unless its an array of strings. I've modified the fingerprint value to be an array of strings.

Here is my setup and some of the JSON payloads to make things a little clearer.

Sentry 8.22.0
Raven 3.23.1
Config: {stacktrace: true}

before the fixes: (notice the ["{{ default }}"] fingerprint)
screen shot 2018-03-05 at 1 05 22 pm

after including the fingerprint as a string: (note the fingerprint and the errors)
screen shot 2018-03-05 at 4 44 44 pm

after including the fingerprint as an array:
screen shot 2018-03-05 at 4 44 14 pm

When stracktrace:true is set via globalOptions there is an attempt to
create a default fingerprint value but it gets dropped from the request to
sentry. This test ensures the fingerprint value is included in the data
sent to sentry.
@jcruz2us jcruz2us requested a review from kamilogorek as a code owner March 6, 2018 03:06
@jcruz2us jcruz2us force-pushed the fix-default-fingerprinting branch from 9278aba to b6c6ff2 Compare March 6, 2018 03:24
@jcruz2us jcruz2us force-pushed the fix-default-fingerprinting branch from b6c6ff2 to bce9f86 Compare March 6, 2018 03:25
@trostli
Copy link

trostli commented Mar 6, 2018

👍

@kamilogorek
Copy link
Contributor

Thanks for contributing to our lovely codebase! :)

Took your changes, made some stylistic changes and unified some code, made sure it works and merged manually to master (still as yours commit of course) - 8f8a624

Will release it next week.

Cheers! :)

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.

3 participants