Release v6.15.0 #614
Merged
Release v6.15.0 #614
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This also uses 'to_a' to ensure the order of code is correct. Hashes are compared without regard for order because usually the order doesn't matter. I don't think the API cares about the order because we explicitly number the lines, but it's useful to validate our code is doing what is expected
We now open each file in a stacktrace once to save opening the same file
multiple times if it is in the stacktrace multiple times. It's quite
common to have the same file in multiple stacktrace lines, e.g. when
a private method raises. The performance gain is especially noticeable
when a a really long file is repeated in the stacktrace as we have to
iterate through each line to find the ones we're interested in
e.g. a trace may look like this:
'/some/file line 5 in "foo"'
'/some/file line 10 in "bar"'
'/some/file line 15 in "baz"'
Previously we would:
1. Open the file for frame 1
2. Read each line up to line 11
3. Trim lines down to lines 2-8
4. Open the file for frame 2
5. Read each line up to line 14
6. Trim lines down to lines 7-13
7. Open the file for frame 3
8. Read each line up to line 19
9. Trim lines down to lines 12-18
Now we:
1. Open the file once
2. Read from line 1 until line 19
3. Add the lines each frame cares about:
a. Lines 1-11 for frame 1
b. Lines 3-14 for frame 2
c. Lines 8-19 for frame 3
3. Trim lines down:
a. To lines 2-8 for frame 1
b. To lines 7-13 for frame 2
c. To lines 12-18 for frame 3
Trimming is necessary because we need to record more lines than we
actually want to keep, so that we can always record 7 lines of context,
even when the frame starts at line 1 or the last line in the file
The new way is still quite inefficient and requires iterating over the
stack frames multiple times, however this passes all tests and can be
improved after profiling it against the old method
There's no Stacktrace.new anymore, so we get one less frame
These will end up being converted into unreadable paths, e.g. "spec/fixtures/crashes/file1.rb" is readable from the root of bugsnag-ruby but will be stripped to "file1.rb" by the project_root, which is unreadable as it's relative to a different directory This test ensures we read files based on the expanded file path, rather than the final path we send to the Bugsnag API
This exposed a bug in the library, which may have never been reported because it was never hit or because it's quite subtle (the stacktrace would be truncated, but that may not be obvious) Previously we were using 'return' which returns from the method, not the 'map' so we'd remove the entire stacktrace if one of the files ended up empty It's possible this condition is never hit and therefore no one has encountered the bug to report it, but it's easy enough to fix and now there's a test to ensure it does the right thing
Most other notifiers set 'code' to null/nil in this case and there's not really a reason not to. If we omit it entirely we _could_ break someone's code if they assume the key 'code' always exists
:s I think this used to make sense in one permutation, but: 11 / 2 == 5.5 ceil(5.5) == 6 6 + 1 == 7 So we were previously using MAXIMUM_LINES_TO_KEEP in a very obsfucated way...
This is largely covered by 'stacktrace_spec', but these tests can be a bit more specific
These were required when Stacktrace was a class and needed to be 'new'-ed, but now it returns an array already
This is no longer needed because, other than misconfiguration, there's no way for the filename to be empty here it was implemented in a479b6e, which also let users add their own filters for the stacktrace file name. This would have been necessary then, but seems like it's not anymore — the only way to trigger this in a real backtrace is by setting the project root to a filename, which isn't a realistic use-case See #604 (review)
These new 'on_error' callbacks will ultimately be a replacement for
the existing 'before_notify_callbacks'
The main problem these solve is that they are global, rather than
being scoped to the current thread. This means you can add a callback
in a Rails initializer and it will run as expected. With the current
'before_notify_callbacks', they only run in the same thread as they
are added and so won't run if created in an initializer. This is a
problem in most (all ?) frameworks, not just Rails
This adds some complexity to the MiddlewareStack, because it now has
to handle procs being passed as middleware. It's important that we
don't wrap the procs earlier for two reasons:
1. We need to be able to remove them, which is much simpler if
we can do this by object identity (because the current method
works that way)
2. We need to be able to abort the 'queue' of callbacks, if one
returns false. This is much easier if we have a list of procs
to run
Add 'on_error' callbacks, to eventually replace 'before_notify_callbacks'
This helps reduce the memory overhead of Bugsnag when session tracking isn't enabled by ~4 MiB
Create the session tracker if sessions are enabled to avoid the overhead of creating it on the first request By requiring the concurrent gem in the session tracker's initialize method, we shifted the overhead from startup to the first request. This caused the first request to go from ~10ms to ~70ms (in the Sinatra example app run locally), which is quite alot of overhead
Fix typo in example Rails 6 app
…ovements Code extraction performance improvements
Reduce the memory overhead of Bugsnag when session tracking isn't enabled
kattrali
approved these changes
Jul 27, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
6.15.0 (27 July 2020)
Enhancements
Add
on_errorcallbacks to replacebefore_notify_callbacks| #608
Improve performance when extracting code from files in stacktraces
| #604
Reduce memory use when session tracking is disabled
| #606
Deprecated
before_notify_callbackshave been deprecated in favour ofon_errorand will be removed in the next major release