-
Notifications
You must be signed in to change notification settings - Fork 3
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
Merge should_capture into before_send #196
Conversation
6e02919
to
1c68736
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks fine, I think we should add more reasoning to the commit message about why we're making this change, and add an entry to the CHANGELOG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Ben - it's shaping up nicely. Just had a few questions/suggestions before we merge:
e00c9f2
to
ba1b7f5
Compare
Will squash the commits together once it's otherwise ready but it seemed easier to keep track of like this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Ben - this is looking good. I've got some suggestions where we might want to increase some test coverage, which would enable us to refactor the code with more confidence. 🤞 Also spotted something major that I should have stipulated in the Trello card - apologies!
let(:configuration) { GovukError::Configuration.new(Raven.configuration) } | ||
|
||
it "ignores errors if they happen in an environment we don't care about" do | ||
ClimateControl.modify SENTRY_CURRENT_ENV: "some-temporary-environment" do | ||
configuration.active_sentry_environments << "production" | ||
expect(configuration.should_capture.call(StandardError.new)).to eq(false) | ||
expect(configuration.before_send.call(StandardError.new)).to be_nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see - because it needs to return nil
to drop the event: https://docs.sentry.io/platforms/ruby/configuration/filtering/
Could you explain that in the commit message when you come to rebase this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Might be worth fleshing out the commit message slightly by linking to the URL above ^ (https://docs.sentry.io/platforms/ruby/configuration/filtering/)
7ba7f54
to
54e3c15
Compare
087451e
to
7f34614
Compare
4e33da5
to
34b94ad
Compare
@@ -12,7 +12,7 @@ def initialize(_raven_configuration) | |||
@data_sync = GovukDataSync.new(ENV["GOVUK_DATA_SYNC_PERIOD"]) | |||
self.active_sentry_environments = [] | |||
self.data_sync_excluded_exceptions = [] | |||
self.before_send = ->(error_or_event, _hint) { error_or_event } | |||
super.before_send = default_before_send_actions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Genius! 👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work, @benjamineskola - the approach looks good and I think we have enough test coverage to be confident it works 🎉
I think this is now ready for a rebase/tidy-up before a final review.
34b94ad
to
589b7aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Ben, sorry for not spotting that this had been rebased.
I've spotted a couple more minor things on final review, which I think would be worth incorporating before we merge 👍 it's looking really great now though.
let(:configuration) { GovukError::Configuration.new(Raven.configuration) } | ||
|
||
it "ignores errors if they happen in an environment we don't care about" do | ||
ClimateControl.modify SENTRY_CURRENT_ENV: "some-temporary-environment" do | ||
configuration.active_sentry_environments << "production" | ||
expect(configuration.should_capture.call(StandardError.new)).to eq(false) | ||
expect(configuration.before_send.call(StandardError.new)).to be_nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Might be worth fleshing out the commit message slightly by linking to the URL above ^ (https://docs.sentry.io/platforms/ruby/configuration/filtering/)
8cf8e18
to
d2263fd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! 👏
f288785
to
ab70628
Compare
This is necessary because `should_capture` is deprecated, but requires slight behaviour changes in order to meet the expectations of both — `should_capture` returns a boolean, but `before_send` returns nil if the error is to be dropped; we want to support both the previous `should_capture` behaviour (filtering) and the existing `before_send` usecase of incrementing statsd counters. In the process we also want to ensure that newly-added callbacks are all run in order, that the statsd counters are incremented once and only once, and that a callback returning nil prevents the final callback from incrementing the counters. `before_send` filtering behaviour is documented at https://docs.sentry.io/platforms/ruby/configuration/filtering/
ab70628
to
b353536
Compare
This was changed in #196.
In #196, we changed `should_capture` to `before_send` in preparation for [the removal of the `should_capture` method](https://github.com/getsentry/sentry-ruby/blob/master/MIGRATION.md#removed) in sentry-ruby. Because we already had some `before_send` logic, and it wasn't idempotent (i.e. it must only be evaluated once, as it increments counters and we want to avoid double-counting), we had to re-architect how the default behaviour is merged with any custom behaviour the downstream developer may set. Since #196 was merged, we've seen errors occuring in Sentry, which should be being ignored via the datasync logic. This suggests the lambdas are not being evaluated. I believe the removal of `before_send=` in configure.rb in https://github.com/alphagov/govuk_app_config/pull/196/files#diff-5617fa12f2b7ce8393099a22ed6ae89a20e286273e17190f2bf9ec48ef5b46a3L2 has meant some of our setup code would not be reached, unless the downstream app calls` before_send=` (and none of them do so far). I also think the override of the `before_send =` on `super` may be causing problems. I've reverted to the previous architecture, whilst keeping the new method names, to use an invocation of `super` that we know was once working. As a caveat, we've had to pass a harmless lambda in configure.rb, to ensure the necessary setup code is called.
In #196, we changed `should_capture` to `before_send` in preparation for [the removal of the `should_capture` method](https://github.com/getsentry/sentry-ruby/blob/master/MIGRATION.md#removed) in sentry-ruby. Because we already had some `before_send` logic, and it wasn't idempotent (i.e. it must only be evaluated once, as it increments counters and we want to avoid double-counting), we had to re-architect how the default behaviour is merged with any custom behaviour the downstream developer may set. Since #196 was merged, we've seen errors occuring in Sentry, which should be being ignored via the datasync logic. This suggests the lambdas are not being evaluated. I believe the removal of `before_send=` in configure.rb in https://github.com/alphagov/govuk_app_config/pull/196/files#diff-5617fa12f2b7ce8393099a22ed6ae89a20e286273e17190f2bf9ec48ef5b46a3L2 has meant some of our setup code would not be reached, unless the downstream app calls` before_send=` (and none of them do so far). I also think the override of the `before_send =` on `super` may be causing problems. I've reverted to the previous architecture, whilst keeping the new method names, to use an invocation of `super` that we know was once working. As a caveat, we've had to pass a harmless lambda in configure.rb, to ensure the necessary setup code is called.
Since #196 and #197, we're seeing errors in Sentry during the data sync where we have configured these to be ignored. Example: https://sentry.io/organizations/govuk/issues/2410319407/?project=202210&referrer=slack The exception chain in the above Sentry error contains `ActiveRecord::StatementInvalid` and `PG::UndefinedTable`. We have `PG::Error` defined as a datasync-ignorable exception by default. [`PG::UndefinedTable` has `PG::Error` as an ancestor](https://github.com/ged/ruby-pg/blob/00cb2ecfaa70470234ee83efbd942b5e6ea0f4ea/spec/pg_spec.rb#L35-L39), so it should be ignoring the exception. To determine whether the exception chain introspection is broken, we're explicitly adding these exception types to the ignore list. If the errors continue to occur, then the issue is elsewhere. For the record, I've also SSH'd into the machine on Integration and ran `cat /etc/govuk/env.d/GOVUK_DATA_SYNC_PERIOD`, which has a value of "22:0-8:0". This, despite its dodgy formatting, does get interpreted correctly by GovukDataSync, therefore I am fairly certain that [`data_sync.in_progress?`](https://github.com/alphagov/govuk_app_config/blob/2616f571fdb28d1ee78c8983b257c16fc64a4994/lib/govuk_app_config/govuk_error/configuration.rb#L45) returns `true` when called during the data sync.
Since [govuk_app_config#196](alphagov/govuk_app_config#196) and [govuk_app_config#197](alphagov/govuk_app_config#197), we're seeing errors in Sentry during the data sync, where we have configured these to be ignored. Example: https://sentry.io/organizations/govuk/issues/2410319407/?project=202210&referrer=slack The exception chain in the above Sentry error contains `ActiveRecord::StatementInvalid` and `PG::UndefinedTable`. We have `PG::Error` defined as a datasync-ignorable exception by default. [`PG::UndefinedTable` has `PG::Error` as an ancestor](https://github.com/ged/ruby-pg/blob/00cb2ecfaa70470234ee83efbd942b5e6ea0f4ea/spec/pg_spec.rb#L35-L39), so it should be ignoring the exception. To determine whether the exception chain introspection is broken, we're explicitly adding these exception types to the ignore list. If the errors continue to occur, then the issue is elsewhere. For the record, I've also SSH'd into the machine on Integration and ran `cat /etc/govuk/env.d/GOVUK_DATA_SYNC_PERIOD`, which has a value of "22:0-8:0". This, despite its dodgy formatting, does get interpreted correctly by GovukDataSync, therefore I am fairly certain that [`data_sync.in_progress?`](https://github.com/alphagov/govuk_app_config/blob/2616f571fdb28d1ee78c8983b257c16fc64a4994/lib/govuk_app_config/govuk_error/configuration.rb#L45) returns `true` when called during the data sync.
In the `before_send` callbacks, the first parameter is of type `Sentry::Event`, and the second parameter (`hint`) is a hash which contains the original exception at key `exception:`. Our datasync-ignorable-exception logic relies on querying the exception chain to see if any of the exceptions in the chain are of an ignorable exception type. This logic doesn't work when we view the `Sentry::Event` object (as we have been doing) - we must instead look at the `hint[:exception]` object. The consequence of this bug was that our logic to ignore certain exceptions that occur during the datasync, no longer worked, and we've therefore seen many thousands of Sentry errors since #196 was merged. However, it wasn't as simple as just checking the `hint` parameter. The `hint` parameter was missing, due to the way we had constructed our tests. Sentry automatically provides the `hint` parameter, but only if the event has been sent through all of the layers of Sentry's architecture, which took a bit of trial and error to get right. I took inspiration from [hub_spec.rb] and [sentry_spec.rb] from the Sentry gem repository. [hub_spec.rb]: https://github.com/getsentry/sentry-ruby/blob/e4b6d66612d9ce601596c23a5dd0d035c559d980/sentry-ruby/spec/sentry/hub_spec.rb [sentry_spec.rb]: https://github.com/getsentry/sentry-ruby/blob/ace176cfceaefc91e29364a1c4180a67337e8ab0/sentry-ruby/spec/sentry_spec.rb This has also exposed how our GovukStatsd incrementing was not working, as it would log all errors as `error_types.sentry_event` rather than their original exception types. This is fixed too.
In the `before_send` callbacks, the first parameter is of type `Sentry::Event`, and the second parameter (`hint`) is a hash which contains the original exception at key `exception:`. Our datasync-ignorable-exception logic relies on querying the exception chain to see if any of the exceptions in the chain are of an ignorable exception type. This logic doesn't work when we view the `Sentry::Event` object (as we have been doing) - we must instead look at the `hint[:exception]` object. The consequence of this bug was that our logic to ignore certain exceptions that occur during the datasync, no longer worked, and we've therefore seen many thousands of Sentry errors since #196 was merged. However, it wasn't as simple as just checking the `hint` parameter. The `hint` parameter was missing, due to the way we had constructed our tests. Sentry automatically provides the `hint` parameter, but only if the event has been sent through all of the layers of Sentry's architecture, which took a bit of trial and error to get right. I took inspiration from [hub_spec.rb] and [sentry_spec.rb] from the Sentry gem repository. [hub_spec.rb]: https://github.com/getsentry/sentry-ruby/blob/e4b6d66612d9ce601596c23a5dd0d035c559d980/sentry-ruby/spec/sentry/hub_spec.rb [sentry_spec.rb]: https://github.com/getsentry/sentry-ruby/blob/ace176cfceaefc91e29364a1c4180a67337e8ab0/sentry-ruby/spec/sentry_spec.rb This has also exposed how our GovukStatsd incrementing was not working, as it would log all errors as `error_types.sentry_event` rather than their original exception types. This is fixed too.
In the `before_send` callbacks, the first parameter is of type `Sentry::Event`, and the second parameter (`hint`) is a hash which contains the original exception at key `exception:`. Our datasync-ignorable-exception logic relies on querying the exception chain to see if any of the exceptions in the chain are of an ignorable exception type. This logic doesn't work when we view the `Sentry::Event` object (as we have been doing) - we must instead look at the `hint[:exception]` object. The consequence of this bug was that our logic to ignore certain exceptions that occur during the datasync, no longer worked, and we've therefore seen many thousands of Sentry errors since #196 was merged. However, it wasn't as simple as just checking the `hint` parameter. The `hint` parameter was missing, due to the way we had constructed our tests. Sentry automatically provides the `hint` parameter, but only if the event has been sent through all of the layers of Sentry's architecture, which took a bit of trial and error to get right. I took inspiration from [hub_spec.rb] and [sentry_spec.rb] from the Sentry gem repository. [hub_spec.rb]: https://github.com/getsentry/sentry-ruby/blob/e4b6d66612d9ce601596c23a5dd0d035c559d980/sentry-ruby/spec/sentry/hub_spec.rb [sentry_spec.rb]: https://github.com/getsentry/sentry-ruby/blob/ace176cfceaefc91e29364a1c4180a67337e8ab0/sentry-ruby/spec/sentry_spec.rb This has also exposed how our GovukStatsd incrementing was not working, as it would log all errors as `error_types.sentry_event` rather than their original exception types. This is fixed too.
In the `before_send` callbacks, the first parameter is of type `Sentry::Event`, and the second parameter (`hint`) is a hash which contains the original exception at key `exception:`. Our datasync-ignorable-exception logic relies on querying the exception chain to see if any of the exceptions in the chain are of an ignorable exception type. This logic doesn't work when we view the `Sentry::Event` object (as we have been doing) - we must instead look at the `hint[:exception]` object. The consequence of this bug was that our logic to ignore certain exceptions that occur during the datasync, no longer worked, and we've therefore seen many thousands of Sentry errors since #196 was merged. However, it wasn't as simple as just checking the `hint` parameter. The `hint` parameter was missing, due to the way we had constructed our tests. Sentry automatically provides the `hint` parameter, but only if the event has been sent through all of the layers of Sentry's architecture, which took a bit of trial and error to get right. I took inspiration from [hub_spec.rb] and [sentry_spec.rb] from the Sentry gem repository. [hub_spec.rb]: https://github.com/getsentry/sentry-ruby/blob/e4b6d66612d9ce601596c23a5dd0d035c559d980/sentry-ruby/spec/sentry/hub_spec.rb [sentry_spec.rb]: https://github.com/getsentry/sentry-ruby/blob/ace176cfceaefc91e29364a1c4180a67337e8ab0/sentry-ruby/spec/sentry_spec.rb This has also exposed how our GovukStatsd incrementing was not working, as it would log all errors as `error_types.sentry_event` rather than their original exception types. This is fixed too.
https://trello.com/c/13nqv4jy/2474-3-merge-govukerrors-shouldcapture-method-into-beforesend-🍐