Fix the new before_send behaviour & tests, and add documentation
#197
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
Running `bundle exec rspec spec/govuk_error/configuration_spec.rb` resulted in errors where `GovukStatsd` wasn't defined. I've now moved this stub to the top of the test file and indicated that it has a `increment` method. I've also fixed the return value of `nil` in configuration.rb, since this value is only used on line 45 as a boolean, so it's good that we return an explicit `false`. (The return value of `nil` is what we want from the _lambda_ return values, but not this internal bit of logic).
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.
ChrisBAshton
force-pushed
the
fix-datasync
branch
from
dedc2bd to
2616f57
Compare
ChrisBAshton
changed the title
before_send tests and documentationbefore_send behaviour & tests, and add documentation
brucebolt
approved these changes
May 21, 2021
ChrisBAshton
added a commit
that referenced
this pull request
Jun 1, 2021
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.
ChrisBAshton
added a commit
to alphagov/bouncer
that referenced
this pull request
Jun 1, 2021
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.
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.
Follow-on from #196. See commits for details.
Trello: https://trello.com/c/13nqv4jy/2474-3-merge-govukerrors-shouldcapture-method-into-beforesend-%F0%9F%8D%90