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

Remove ActiveJob keys for both Sidekiq and DelayedJob #898

Merged
merged 1 commit into from
Sep 21, 2020
Merged

Remove ActiveJob keys for both Sidekiq and DelayedJob #898

merged 1 commit into from
Sep 21, 2020

Conversation

alissonbrunosa
Copy link
Contributor

@alissonbrunosa alissonbrunosa commented Apr 17, 2019 •
edited
Loading

It's as same as #386 but for both Sidekiq and DelayedJob

Once an ActiveJob is queued, ActiveRecord references get serialized into
some internal reserved keys, such as _aj_globalid.
The problem is, if this job in turn gets queued back into ActiveJob with
these magic reserved keys, ActiveJob will throw up and error. We want to
capture these and mutate the keys so we can sanely report it.

The difference is instead of rename the key I removed it, because this key will not be used.

@alissonbrunosa alissonbrunosa force-pushed the fix-384-for-sidekiq-and-delayedjob branch from aee5baf to 3221522 Compare April 17, 2019 22:31
@cantino
Copy link

cantino commented Jun 5, 2019

Looks like a minor Rubocop issue on this.

@st0012 st0012 added this to the 3.1.0 milestone Aug 21, 2020
st0012
st0012 requested changes Sep 10, 2020
View reviewed changes
Copy link
Collaborator

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

Thanks for the work! It looks good in general, just added some small suggestions. Can you rebase it against master as well?

lib/raven/integrations/delayed_job.rb Outdated Show resolved Hide resolved
spec/raven/integrations/sidekiq_spec.rb Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (v3-1-stable@e05c8c5). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##             v3-1-stable     #898   +/-   ##
==============================================
  Coverage               ?   98.17%           
==============================================
  Files                  ?       57           
  Lines                  ?     2466           
  Branches               ?        0           
==============================================
  Hits                   ?     2421           
  Misses                 ?       45           
  Partials               ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e05c8c5...44979cd. Read the comment docs.

alissonbrunosa
alissonbrunosa commented Sep 17, 2020
View reviewed changes
lib/raven/context_filter.rb Outdated
private

def filter_context_hash(key, value)
(key = key[3..-1]) if key[0..3] == ACTIVEJOB_RESERVED_PREFIX
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will remove only the first 3 chars from the key. Like so _aj_symbol_keys => _symbol_keys. Should I remove this _ in the beginning as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we do it with substitution instead? like

ACTIVEJOB_RESERVED_PREFIX_REGEX = /^_aj_/.freeze

key = key.sub(ACTIVEJOB_RESERVED_PREFIX_REGEX, "") if key.match?(ACTIVEJOB_RESERVED_PREFIX_REGEX)

st0012
st0012 requested changes Sep 17, 2020
View reviewed changes
Copy link
Collaborator

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

overall looks good 👍 could be merged into 3.1, just need some updates

lib/raven/context_filter.rb Outdated
private

def filter_context_hash(key, value)
(key = key[3..-1]) if key[0..3] == ACTIVEJOB_RESERVED_PREFIX
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we do it with substitution instead? like

ACTIVEJOB_RESERVED_PREFIX_REGEX = /^_aj_/.freeze

key = key.sub(ACTIVEJOB_RESERVED_PREFIX_REGEX, "") if key.match?(ACTIVEJOB_RESERVED_PREFIX_REGEX)

lib/raven/context_filter.rb Outdated Show resolved Hide resolved
@st0012 st0012 modified the milestones: 3.1.0, 3.1.1 Sep 17, 2020
@st0012 st0012 changed the base branch from master to v3-1-stable September 18, 2020 16:25
@st0012 st0012 changed the base branch from v3-1-stable to master September 18, 2020 16:26
@st0012 st0012 changed the base branch from master to v3-1-stable September 18, 2020 16:30
@st0012
Copy link
Collaborator

st0012 commented Sep 18, 2020

@alissonbrunosa sorry that I forgot to change the base of this PR 🤦‍♂️ because we have started the restructuring work of version 4.0, all the previously-opened contributions should be merged into v3-1-stable.

so I wonder if you can revert your merge of the master and merge/rebase v3-1-stable instead? I've tried squash merging this PR locally, but that'll erase you from the merge commit (we do squash commit). very sorry for this 🙇

@alissonbrunosa
- Move files to Utils namespace
44979cd 
- Add spec for context filter
- Use context filter for both Sidekiq and DelayedJOb
- fixing specs
@alissonbrunosa alissonbrunosa force-pushed the fix-384-for-sidekiq-and-delayedjob branch from f8ab981 to 44979cd Compare September 21, 2020 09:52
@alissonbrunosa
Copy link
Contributor Author

alissonbrunosa commented Sep 21, 2020 •
edited
Loading

@st0012 don't worry 😅
I tried to revert the merge commit, but it made things way worse.
So, I created a branch from v3-1-stable, and add the changes.

@st0012
Copy link
Collaborator

st0012 commented Sep 21, 2020

@alissonbrunosa thanks for the work and sorry again for the hassles 😅

@st0012 st0012 merged commit f022ca4 into getsentry:v3-1-stable Sep 21, 2020
@alissonbrunosa alissonbrunosa deleted the fix-384-for-sidekiq-and-delayedjob branch September 21, 2020 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@st0012 st0012 st0012 approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
3.1.1
Development

Successfully merging this pull request may close these issues.
4 participants
@alissonbrunosa @cantino @codecov-commenter @st0012