-
-
Notifications
You must be signed in to change notification settings - Fork 494
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
Add monotonic_active_support_logger #1531
Conversation
360eaa7
to
e74f1bb
Compare
sentry-rails/spec/sentry/rails/active_support_breadcrumbs_spec.rb
Outdated
Show resolved
Hide resolved
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@morissetcl thanks for the proposal. I didn't know about the 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: |
@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. |
Ah that's a good catch. Thank you 👍 |
a1c37dd
to
d80913f
Compare
sentry-rails/spec/sentry/rails/breadcrumbs/active_support_breadcrumbs_spec.rb
Outdated
Show resolved
Hide resolved
ef60964
to
3096d72
Compare
@morissetcl oops, looks like #1535 caused an conflict on this. can you fix it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 👍
3096d72
to
31fd4ef
Compare
31fd4ef
to
12d7c71
Compare
Hi,
This is a proposal to support
monotonic_time
.The regular
subscriber
usereal_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 agoTwo years ago Rails add amonotonic_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.