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

Support changing Event's api key in OnErrorCallback #928

Merged
merged 3 commits into from
Sep 10, 2020

Conversation

fractalwrench
Copy link
Contributor

@fractalwrench fractalwrench commented Sep 3, 2020

Goal

Supports changing the apiKey for an Event via an OnErrorCallback. This is achieved by encoding the apiKey in the filename if an event is cached on disk, and reading this apiKey from the filename at the point of delivery.

Changeset

  • DeliveryDelegate uses the apiKey set on Event rather than Configuration, which covers delivery of handled errors
  • errorApiHeaders() now takes the apiKey as a parameter when generating HTTP headers so that Bugsnag-Api-Key is not always set to config.apiKey
  • flushEventFile() reads the apiKey from the filename if present, and otherwise falls back to the config.apiKey value if the cached file is in the legacy format
  • Fixed a minor bug where 'not-jvm' was always added to a filename if the report was not a startup crash. The presence of 'not-jvm' has no apparent consequence on the notifier's functionality and appears to be used only for debugging purposes
  • Encoded the apiKey in the filename

This changeset does not address altering the API key for NDK errors, which will be handled separately.

Testing

Added an E2E test for a handled error (which should be delivered without writing to disk) and an unhandled error (which requires writing to disk). Additionally unit tests have been added to cover filename generation for each event, and reading the apiKey from the filename.

@fractalwrench fractalwrench changed the title fix: support changing api key in event callback Support changing Event's api key in OnErrorCallback Sep 3, 2020
@bugsnagbot
Copy link
Collaborator

bugsnagbot commented Sep 3, 2020

Android notifier sizes

Format Size impact of Bugsnag (kB) Size impact of Bugsnag when Minified (kB)
APK 1437.77 1358.8
arm64_v8a 369.24 287.33
armeabi 348.77 266.85
armeabi_v7a 328.29 250.47
x86 406.1 324.19
x86_64 389.72 307.81

Generated by 🚫 Danger

@fractalwrench fractalwrench marked this pull request as ready for review September 3, 2020 14:57
Copy link
Contributor

@tomlongridge tomlongridge left a comment

Choose a reason for hiding this comment

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

A few comments, but broadly looks good.

Can you create an integration branch and target that so we can combine the session and NDK work too?

@fractalwrench fractalwrench changed the base branch from next to event-api-key-change September 9, 2020 15:17
@fractalwrench fractalwrench merged commit aa9994f into event-api-key-change Sep 10, 2020
@fractalwrench fractalwrench deleted the PLAT-4610/event-api-key branch September 10, 2020 09:08
init {
config.autoTrackSessions = false
config.addOnError(OnErrorCallback { event ->
event.apiKey = "0000111122223333aaaabbbbcccc9999"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: might be worth adding another scenario where one client instance generates an event with a specific API key (without overriding it in OnErrorCallback, just using its default config) and dumps it on the file system and then a new client instance is created with a different API key, and that new client instance ends up uploading the error and uses the right (initial) API key.

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

4 participants