Skip to content

Add monotonic_active_support_logger#1531

Merged
st0012 merged 4 commits intogetsentry:masterfrom
morissetcl:add-monotonic-time-in-configuration
Aug 16, 2021
Merged

Add monotonic_active_support_logger#1531
st0012 merged 4 commits intogetsentry:masterfrom
morissetcl:add-monotonic-time-in-configuration

Conversation

@morissetcl
Copy link
Copy Markdown
Contributor

@morissetcl morissetcl commented Aug 13, 2021 •
edited
Loading

Hi,

This is a proposal to support monotonic_time.

The regular subscriber use real_time and can jump forwards and backwards as the system time-of-day clock is changed. It can be confusing, especially when computing elapsed time.

Few years ago Two years ago Rails add a monotonic_subscriber for instrumentation, I think it will be great to add the ability to set it.

There is other places in the code where it could be propagated. (especially for tracing)
Just would like have your feelings about this option and if it could make sense for you.

BTW thanks for maintaining this gem it's really useful.

@morissetcl morissetcl force-pushed the add-monotonic-time-in-configuration branch from 360eaa7 to e74f1bb Compare August 13, 2021 20:12
morissetcl
morissetcl commented Aug 13, 2021
View reviewed changes
@morissetcl morissetcl changed the title Add monotonic time in configuration Add enabled_monotonic_time configuration Aug 13, 2021
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 14, 2021 •
edited
Loading

Codecov Report

Merging #1531 (1b2f2e6) into master (d101202) will increase coverage by 0.06%.
The diff coverage is 70.33%.

❗ Current head 1b2f2e6 differs from pull request most recent head c455276. Consider uploading reports for the commit c455276 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1531      +/-   ##
==========================================
+ Coverage   98.21%   98.27%   +0.06%     
==========================================
  Files         218      126      -92     
  Lines       10624     6902    -3722     
==========================================
- Hits        10434     6783    -3651     
+ Misses        190      119      -71
Impacted Files Coverage Δ
sentry-rails/lib/sentry/rails/railtie.rb 96.72% <50.00%> (-3.28%) ⬇️
...rumbs/monotonic_active_support_breadcrumbs_spec.rb 52.94% <52.94%> (ø)
...ails/breadcrumb/monotonic_active_support_logger.rb 55.00% <55.00%> (ø)
...ils/breadcrumbs/active_support_breadcrumbs_spec.rb 100.00% <100.00%> (ø)
sentry-rails/lib/sentry/rails/active_job.rb 97.05% <0.00%> (-0.09%) ⬇️
sentry-rails/lib/sentry/rails/configuration.rb 100.00% <0.00%> (ø)
...-sidekiq/spec/sentry/sidekiq/error_handler_spec.rb 100.00% <0.00%> (ø)
...ven/spec/raven/utils/exception_cause_chain_spec.rb
sentry-raven/lib/raven/transports.rb
sentry-raven/spec/raven/processors/cookies_spec.rb
... and 95 more

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 d101202...c455276. Read the comment docs.

@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Aug 14, 2021

@morissetcl thanks for the proposal. I didn't know about the monotonic feature until now!

but it looks like this feature is only added after Rails 6.1 (commit: rails/rails@93b652a). and having a global config only for a certain Rails version is confusing.

so I want to propose another idea: you can add a new breadcrumb logger: monotonic_active_support_logger for this support (which will print warnings + not being activated if the client's Rails version < 6.1). and the user can just use the config.breadcrumb_loggers option for this feature too.

@morissetcl
Copy link
Copy Markdown
Contributor Author

morissetcl commented Aug 14, 2021

@st0012 hmm excellent point. I'm going to make these changes.

What about #1531 (comment) ? Any thought ? If changes are necessary I propose to do them in a separate PR.

@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Aug 14, 2021

What about #1531 (comment) ? Any thought ? If changes are necessary I propose to do them in a separate PR.

Ah that's a good catch. Thank you 👍

@morissetcl morissetcl force-pushed the add-monotonic-time-in-configuration branch 2 times, most recently from a1c37dd to d80913f Compare August 14, 2021 21:46
@morissetcl morissetcl changed the title Add enabled_monotonic_time configuration Add monotonic_active_support_logger Aug 14, 2021
@morissetcl morissetcl mentioned this pull request Aug 15, 2021
Merged
@st0012 st0012 added this to the 4.7.0 milestone Aug 15, 2021
@morissetcl morissetcl force-pushed the add-monotonic-time-in-configuration branch from ef60964 to 3096d72 Compare August 15, 2021 17:24
@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Aug 16, 2021

@morissetcl oops, looks like #1535 caused an conflict on this. can you fix it?

st0012
st0012 approved these changes Aug 16, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@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.

Thank you 👍

@morissetcl morissetcl force-pushed the add-monotonic-time-in-configuration branch from 3096d72 to 31fd4ef Compare August 16, 2021 10:55
@morissetcl morissetcl force-pushed the add-monotonic-time-in-configuration branch from 31fd4ef to 12d7c71 Compare August 16, 2021 11:01
@st0012 st0012 merged commit fb7a0c3 into getsentry:master Aug 16, 2021
@sl0thentr0py sl0thentr0py mentioned this pull request Sep 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@st0012 st0012 st0012 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

sentry-rails

Projects

None yet

Milestone

4.7.0

Development

Successfully merging this pull request may close these issues.

3 participants

@morissetcl @codecov-commenter @st0012