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.

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

Avoid leaking tracing timestamp to breadcrumbs #1575

Merged
merged 3 commits into from Sep 17, 2021

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented Sep 17, 2021

Currently when using sentry-rails's active_support_logger breadcrumb logger with tracing enabled, all the breadcrumbs will have an additional start_timestamp attribute. That attribute is added for helping tracing subscribers calculate each span's duration and is not designed to be leaked outside.

So this PR adds 2 changes:

  • Because the attribute is added to all instrument events and is visible to all subscribers (including other libraries or users' subscribers), its name should have sentry_ prefix to avoid misuse.
  • InstrumentPayloadCleanupHelper should remove the sentry_start_timestamp key from breadcrumbs.

@st0012 st0012 added this to the 4.8.0 milestone Sep 17, 2021
@st0012 st0012 self-assigned this Sep 17, 2021
@st0012 st0012 added this to In progress in 4.x via automation Sep 17, 2021
@st0012 st0012 force-pushed the avoid-leaking-tracing-timestamp-to-breadcrumbs branch from f66b336 to b0d3420 Compare September 17, 2021 14:56
@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2021

Codecov Report

Merging #1575 (9c553d4) into master (fddb235) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1575   +/-   ##
=======================================
  Coverage   98.06%   98.06%           
=======================================
  Files         126      126           
  Lines        6923     6932    +9     
=======================================
+ Hits         6789     6798    +9     
  Misses        134      134           
Impacted Files Coverage Δ
...ntry/rails/tracing/action_controller_subscriber.rb 94.44% <ø> (ø)
sentry-rails/lib/sentry/rails.rb 100.00% <100.00%> (ø)
.../sentry/rails/instrument_payload_cleanup_helper.rb 100.00% <100.00%> (ø)
sentry-rails/lib/sentry/rails/tracing.rb 96.87% <100.00%> (+0.10%) ⬆️
...lib/sentry/rails/tracing/action_view_subscriber.rb 100.00% <100.00%> (ø)
...b/sentry/rails/tracing/active_record_subscriber.rb 100.00% <100.00%> (ø)
...ils/breadcrumbs/active_support_breadcrumbs_spec.rb 100.00% <100.00%> (ø)

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 fddb235...9c553d4. Read the comment docs.

@st0012 st0012 merged commit a6ce224 into master Sep 17, 2021
4.x automation moved this from In progress to Done Sep 17, 2021
@st0012 st0012 deleted the avoid-leaking-tracing-timestamp-to-breadcrumbs branch September 17, 2021 15:28
@st0012 st0012 modified the milestones: 4.8.0, 4.7.3 Sep 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4.x
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants