-
-
Notifications
You must be signed in to change notification settings - Fork 490
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
ActiveSupportLogger logs way too many Ruby objects (for SQL breadcrumbs) #1266
Comments
thanks for reporting the issue. I agree that there are many breadcrumb data that are unnecessary to capture, but I don't think we can just use the tracing subscriber's approach here. although tracing subscribers & breadcrumb loggers both subscribe to instrumentation events, they have very different purposes:
I'm not saying it's an approach I'll never take. I just think it could sacrifice some customers the data they need so I won't choose it without trying other options first. Possible Causes of The Errorbut we need to talk about the potential cause of the
My Proposalsso my ideas for addressing the above issues are: Separate individual breadcrumb's serializationwe serialize breadcrumbs individually and ditch the problematic data if we see any error Pros
Cons
Clean the data ourselveswe scrub the data as you did for tracking down this issue (convert all data into pure Ruby objects) + remove circular references Pros
Cons
No Conclusion, YetI actually have implemented the first approach in #1250, which has been released in Update today we released version config.before_breadcrumb = lambda do |breacrumb, hint|
breadcrumb.data = scrubbed_data
breadcrumb
end |
@leonid-shevtsov does your app use the |
Describe the bug
Symptom:
stack level too deep
error when sending eventI've debugged this down to
breadcrumbs
containing Ruby objects that definitely should not be sent with the trace.Here is one such breadcrumb; I've obtained it by patching
event.to_hash
and sanitizing it:To Reproduce
Just enable transaction tracing and active_support_logger. (Transaction tracing needed to generate events)
Expected behavior
I really insist you should not use a blacklist approach with the
IGNORED_DATA_TYPES
constant here. With a blacklist, every new change to rails logging - or instrumentation from any other library - can break breadcrumbs again. It makes more sense to log the bare minimum of information that's known to be simple data types (name, timestamp), and then add to that for certain event types with data that, again, you know to be simple (like a SQL query).BTW performance traces don't have the same problem - and that's because
Sentry::Rails::Tracing::ActiveRecordSubscriber
uses just the approach I described.Environment
Config
The text was updated successfully, but these errors were encountered: