Skip to content

Allow customization breadcrumb data of ActiveSupportLogger#2139

Merged
sl0thentr0py merged 5 commits into
getsentry:masterfrom
igordepolli:master
Oct 24, 2023
Merged

Allow customization breadcrumb data of ActiveSupportLogger#2139
sl0thentr0py merged 5 commits into
getsentry:masterfrom
igordepolli:master

Conversation

@igordepolli

@igordepolli igordepolli commented Oct 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Ref: #2135

Description

Add new config.rails.active_support_logger_subscription_items to allow customization breadcrumb data of active support logger

config.rails.active_support_logger_subscription_items["sql.active_record"] << :type_casted_binds
config.rails.active_support_logger_subscription_items.delete("sql.active_record")
config.rails.active_support_logger_subscription_items["foo"] = :bar

sl0thentr0py
sl0thentr0py requested changes Oct 23, 2023
View reviewed changes

@sl0thentr0py sl0thentr0py left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thx @igordepolli!
one small change then good to go

Comment thread sentry-rails/lib/sentry/rails/breadcrumb/active_support_logger.rb Outdated
@codecov

codecov Bot commented Oct 23, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2139 (c51f9e0) into master (1c0550f) will decrease coverage by 37.58%.
Report is 3 commits behind head on master.
The diff coverage is 75.00%.

@@             Coverage Diff             @@
##           master    #2139       +/-   ##
===========================================
- Coverage   97.27%   59.69%   -37.58%     
===========================================
  Files          97       86       -11     
  Lines        3630     3389      -241     
===========================================
- Hits         3531     2023     -1508     
- Misses         99     1366     +1267
Components Coverage Δ
sentry-ruby 55.88% <ø> (-42.10%) ⬇️
sentry-rails 45.75% <75.00%> (-49.21%) ⬇️
sentry-sidekiq 93.70% <ø> (ø)
sentry-resque 92.06% <ø> (ø)
sentry-delayed_job 94.36% <ø> (ø)
sentry-opentelemetry 100.00% <ø> (ø)
Files Coverage Δ
sentry-rails/lib/sentry/rails/configuration.rb 85.71% <100.00%> (-11.17%) ⬇️
sentry-rails/lib/sentry/rails/railtie.rb 25.00% <0.00%> (-73.69%) ⬇️

... and 73 files with indirect coverage changes

sl0thentr0py
sl0thentr0py reviewed Oct 23, 2023
View reviewed changes
Comment thread CHANGELOG.md Outdated
@igordepolli

igordepolli commented Oct 23, 2023

Copy link
Copy Markdown
Contributor Author

thx @igordepolli! one small change then good to go

@sl0thentr0py Done. Ty for review :)

st0012
st0012 reviewed Oct 23, 2023
View reviewed changes
Comment thread sentry-rails/lib/sentry/rails/configuration.rb
Comment thread sentry-rails/spec/sentry/rails/breadcrumbs/active_support_logger_spec.rb Outdated
@igordepolli

igordepolli commented Oct 23, 2023

Copy link
Copy Markdown
Contributor Author

@st0012 Done. Good catch! Ty! :)

@igordepolli
igordepolli requested a review from st0012 October 23, 2023 23:31
@sl0thentr0py

sl0thentr0py commented Oct 24, 2023

Copy link
Copy Markdown
Member

@igordepolli the ensure fails on ruby 2.4 because this feature was added in 2.5, you can either add a begin/end or just wrap the spec in a context with a before/after which is probably cleaner.

https://github.com/getsentry/sentry-ruby/actions/runs/6620176438/job/17999733560?pr=2139#step:5:156

You can also just give me push rights to the PR and I'll take care of the rest and merge this in.

@igordepolli

igordepolli commented Oct 24, 2023

Copy link
Copy Markdown
Contributor Author

@igordepolli the ensure fails on ruby 2.4 because this feature was added in 2.5, you can either add a begin/end or just wrap the spec in a context with a before/after which is probably cleaner.

https://github.com/getsentry/sentry-ruby/actions/runs/6620176438/job/17999733560?pr=2139#step:5:156

You can also just give me push rights to the PR and I'll take care of the rest and merge this in.

@sl0thentr0py Oh, sorry for the inconvenience. You already have the right to manipulate PR however you want, feel free.

@sl0thentr0py
sl0thentr0py merged commit d315a15 into getsentry:master Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sl0thentr0py sl0thentr0py sl0thentr0py approved these changes
@st0012 st0012 Awaiting requested review from st0012

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@igordepolli @sl0thentr0py @st0012