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

Add monotonic_active_support_logger #1531

Merged

Conversation

morissetcl
Copy link
Contributor

@morissetcl morissetcl commented Aug 13, 2021

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 changed the title Add monotonic time in configuration Add enabled_monotonic_time configuration Aug 13, 2021
@codecov-commenter
Copy link

codecov-commenter commented Aug 14, 2021

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
Collaborator

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
Contributor Author

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

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
@st0012 st0012 added this to In progress in 4.x via automation Aug 15, 2021
@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
Collaborator

st0012 commented Aug 16, 2021

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

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.

Thank you 👍

4.x automation moved this from In progress to Reviewer approved Aug 16, 2021
@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
4.x automation moved this from Reviewer approved to Done Aug 16, 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

3 participants