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: Preserve event id of minidump upload #361

Merged
merged 7 commits into from
Dec 17, 2019

Conversation

untitaker
Copy link
Member

No description provided.

@untitaker untitaker requested a review from jan-auer December 11, 2019 14:58
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this is not sufficient. We also need to parse this from other payloads. Ideally, we would abstract over that, given that we already have this helper in another place. If I remember correctly, then there are the following ways to ingest an event ID with minidumps:

  1. The sentry key containing a JSON-encoded payload
  2. The sentry[event_id] key containing the event ID
  3. The __sentry-event messagepack payload
  4. If we want to support it going forward (did not support in the past), the guid form field
  5. Random generated.

You can see these different extractions in EventProcessor::extract_event.

@untitaker
Copy link
Member Author

Ideally, we would abstract over that

yeah so this is where my confusion starts. We can abstract over it, but then we will probably just pay the cost of parsing everything twice... once in the endpoint and once in the event processing.

@jan-auer
Copy link
Member

we will probably just pay the cost of parsing everything twice

Most likely we have to do that :(
Unfortunately, we cannot rely on the SDK to send the event id in a header.

@untitaker
Copy link
Member Author

@jan-auer do you think we could get away with parsing the event ID only from a few select places and preventing it from being set in others? I.e. we allow sentry[event_id] in formdata but actually show an event error when it is being set through __sentry_event etc

@jan-auer
Copy link
Member

Unfortunately, we have to support both the sentry JSON value and __sentry-event. I don’t think anyone ever sent sentry[event_id], but it’s strange to have that as an exception tbh.

untitaker and others added 5 commits December 16, 2019 17:05
* master:
  fix: Update fixture to produce stable snapshot
  fix: Check in missing snapshot
  ref: Bump sentry-types
  test(general): Add a snapshot for Android
  Revert "feat(general): Add thread.errored attribute (#306)" (#366)
  Revert "doc: Changelog for 0.4.66"
  doc: Changelog for 0.4.66
  fix(server): Make status codes consistent with Sentry (#365)
  release: 0.4.65
  test: Update security report test snapshots
  build: Update redis dependency
  meta: Changelog for 0.4.65
  Revert "ref: Use jemalloc instead of system allocator (#344)" (#360)
  ref: Update user-agent database (#363)
  build: Update rdkafka to replace git dependency (#364)
Copy link
Member Author

@untitaker untitaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If @untitaker approves, I approve.

@jan-auer jan-auer merged commit b115b5b into master Dec 17, 2019
@jan-auer jan-auer deleted the fix/minidump-preserve-event-id branch December 17, 2019 16:16
jan-auer added a commit that referenced this pull request Dec 18, 2019
* master:
  fix(server): Preserve event id of minidump upload (#361)
  fix: Update fixture to produce stable snapshot
  fix: Check in missing snapshot
  ref: Bump sentry-types
  test(general): Add a snapshot for Android
  Revert "feat(general): Add thread.errored attribute (#306)" (#366)
  Revert "doc: Changelog for 0.4.66"
  doc: Changelog for 0.4.66
  fix(server): Make status codes consistent with Sentry (#365)
  release: 0.4.65
  test: Update security report test snapshots
  build: Update redis dependency
  meta: Changelog for 0.4.65
  Revert "ref: Use jemalloc instead of system allocator (#344)" (#360)
  ref: Update user-agent database (#363)
  build: Update rdkafka to replace git dependency (#364)
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.

2 participants