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.

Sign up for GitHub

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

Jump to bottom

Add Device time of report capture to JSON payload #315

Merged
merged 8 commits into from Oct 31, 2018
Merged

Add Device time of report capture to JSON payload #315

merged 8 commits into from Oct 31, 2018

Conversation

fractalwrench
Copy link
Contributor

@fractalwrench fractalwrench commented Oct 23, 2018 •
edited

Goal

Reports may be cached on a device if no network connection is available. In this case, the report may not be delivered for hours, or even days, after the error was originally captured by Bugsnag.

Sending the time at which the report was originally captured allows for timestamps to be normalised. This avoids confusing information similar to the following being shown:

screen shot 2018-10-23 at 16 19 51

Design

The pipeline normalises timestamps based off the device.time field.

Changeset

Serialises the time at which the report was generated into the device.time payload field.

Tests

  • Added mazerunner tests for handled/unhandled errors, payload serialisation unit test
  • Checked payload manually for presence of device.time in handled/unhandled error
  • Checked breadcrumb time is updated in the dashboard when sending in a payload that has been cached and had the timestamp manually edited

Review

For the submitter, initial self-review:

  • Commented on code changes inline explain the reasoning behind the approach
  • Reviewed the test cases added for completeness and possible points for discussion
  • A changelog entry was added for the goal of this pull request
  • Check the scope of the changeset - is everything in the diff required for the pull request?
  • This pull request is ready for:
    • Initial review of the intended approach, not yet feature complete
    • Structural review of the classes, functions, and properties modified
    • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

  • Consistency across platforms for structures or concepts added or modified
  • Consistency between the changeset and the goal stated above
  • Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
  • Usage friction - is the proposed change in usage cumbersome or complicated?
  • Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
  • Concurrency concerns - if components are accessed asynchronously, what issues will arise
  • Thoroughness of added tests and any missing edge cases
  • Idiomatic use of the language

@fractalwrench fractalwrench changed the title Device time Add Device time of report capture to JSON payload Oct 23, 2018
snmaynard
snmaynard reviewed Oct 23, 2018
View reviewed changes
features/handled_errors.feature Outdated
@@ -6,6 +6,7 @@ Scenario: Override errorClass and message from a notifyError() callback
And the request is a valid for the error reporting API
And the exception "errorClass" equals "Bar"
And the exception "message" equals "Foo"
And the payload field "events.0.device.time" is not null
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to verify the format of this field and also that it is the current time.

@fractalwrench
Copy link
Contributor Author

I have updated the mazerunner scenario to check that the field is a timestamp. There's also some coverage in the unit tests for ensuring the timestamp is serialised from the KSCrash field, if that's sufficient?

Note: Cocoa mazerunner scenarios still do not work on my machine and this makes it very hard to write/debug scenarios.

@snmaynard
Copy link
Contributor

It doesnt look like any tests test that the timestamp is the right time (i.e. within 5 seconds of now).

@fractalwrench
Copy link
Contributor Author

I have updated the mazerunner scenario to check that the field is a timestamp within 30s of the current time .

snmaynard
snmaynard suggested changes Oct 30, 2018
View reviewed changes
Copy link
Contributor

@snmaynard snmaynard left a comment

Choose a reason for hiding this comment

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

Once my comment has been addressed I'm ok for there to be a quick review in the UK and then for this to merge into next.

features/steps/ios_steps.rb Outdated
Then("the event {string} is within {int} ms of the current timestamp") do |field, threshold_ms|
value = read_key_path(find_request(0)[:body], "events.0.#{field}")
assert_not_nil(value, "Expected a timestamp")
nowMs = Time.now.to_i
Copy link
Contributor

Choose a reason for hiding this comment

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

These are actually seconds so all these times should be updated to be seconds, and the thresholds in tests should be seconds.

features/steps/ios_steps.rb Outdated
@@ -1,3 +1,5 @@
require 'date'
Copy link
Contributor

Choose a reason for hiding this comment

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

You dont use this require.

kinbiko
kinbiko approved these changes Oct 31, 2018
View reviewed changes
Copy link

@kinbiko kinbiko left a comment

Choose a reason for hiding this comment

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

LGTM

@fractalwrench fractalwrench merged commit 3e718fb into next Oct 31, 2018
@fractalwrench fractalwrench deleted the device-time branch October 31, 2018 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snmaynard snmaynard snmaynard requested changes

@kinbiko kinbiko kinbiko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fractalwrench @snmaynard @kinbiko