Conversation
Right now the Event is updated via the Scope instead of direct options. So those options aren't actually used.
| def add_exception_interface(exc) | ||
| if exc.respond_to?(:sentry_context) | ||
| @extra.merge!(exc.sentry_context) | ||
| end |
There was a problem hiding this comment.
Subtle regression here.
Previously, #sentry_context could return a hash like { extra: …, tags: …, fingerprint: … } so you could enrich every aspect of the event captured by an exception.
Now, the #sentry_context method is treated as extra context only.
There was a problem hiding this comment.
Hey I'm sorry for the trouble. It's an undocumented feature that's ported from sentry-raven, which I thought no body was using (I didn't mention it in the migration guide and didn't receive any report). So I didn't think much when removing it. Would you mind sharing your usage of it?
There was a problem hiding this comment.
Sorry, I should have mentioned sooner. We avoided upgrading when this changed and did not alert you at the time.
We used it to "enrich" exceptions with the relevant Sentry context. Here are some examples:
# Cross-cutting exceptions like database deadlocks and Redis timeouts can
# crop up anywhere. Sentry groups exceptions by backtrace, though, so we
# end up with tons of separate events for the same underlying cause.
# Fix by overriding the exception fingerprint to group them all together.
module FingerprintExceptionByClassName
def raven_context
{ fingerprint: [ self.class.name ] }
end
end
require 'unicorn'
Unicorn::ClientShutdown.include FingerprintExceptionByClassName
Redis::TimeoutError.include FingerprintExceptionByClassName
Faraday::TimeoutError.include FingerprintExceptionByClassName
Faraday::ConnectionFailed.include FingerprintExceptionByClassName
Faraday::ClientError.class_eval do
def raven_context
response = self.response || {}
{ fingerprint: [ self.class.name, response[:status] ],
tags: { response_status: response[:status] },
extra: { response_body: response[:body] }
}
end
end
ActiveRecord::StatementInvalid.class_eval do
def raven_context
case message
when /\A(.*) mysql_errno=(\d+) sqlstate=(\d+)/
{ fingerprint: [ 'Mysql2', $2.to_i ],
tags: { mysql_errno: $2.to_i, sqlstate: $3.to_i }
}
else
defined?(super) ? super : {}
end
end
end
require 'rmagick'
Magick::ImageMagickError.class_eval do
def raven_context
case message
when /Unsupported marker type/
{ fingerprint: [ "Magick: Unsupported marker type" ] }
when /\A(.*)`/
{ fingerprint: [ "Magick: #{$1}" ] }
else
defined?(super) ? super : {}
end
end
end
require 'resque'
Resque::DirtyExit.class_eval do
def raven_context
case message
when /pid (\d+) ([A-Z]+) \(signal (\d+)\)/
{ fingerprint: [ 'Resque::DirtyExit', $3.to_i ],
tags: { pid: $1.to_i, signame: $2, signo: $3.to_i }
}
else
defined?(super) ? super : {}
end
end
end
SignalException.class_eval do
def raven_context
{ fingerprint: [ 'SignalException', signo ],
tags: { signo: signo }
}
end
endThere was a problem hiding this comment.
I think you can replicate the same behavior with an addition to the SDK's before_send callback:
config.before_send = lambda do |event, hint|
if exception = hint[:exception]
exception.raven_context.each do |key, value|
# here I assume the event would be a Sentry::Event object
# it'll be a hash if you use the async callback though (which will be removed in version 5.0)
event.send("#{key}=", value)
end
end
event
endJust curious, if there're other issues that blocked you from migrating to sentry-ruby?
There was a problem hiding this comment.
Ah, that's a nice approach! We did use before_send but didn't consider retaining the existing #raven_context methods.
No other blocker issues. The migration was straightforward enough that I wondered whether providing a Raven compatibility layer with deprecations would be feasible & helpful. Most client usage is fairly basic and most API breakage are simple name changes like Raven.user_context(…) → Raven.set_user(…) and Raven.configure → Sentry.init.
The only other notable bit is that config.tags = … within the Raven.configure block was a nice and obvious way to set default tags for all events. That can be done with Sentry.set_tags(…) at app boot as well so it's achievable, but it's less obvious and discoverable.
/cc @lewispb
There was a problem hiding this comment.
I wondered whether providing a Raven compatibility layer with deprecations would be feasible & helpful.
Some APIs actually have subtle behavioral changes, like Sentry.set_user doesn't take a block anymore. So I thought an extra layer could be confusing at times. Also as you said, most of them are name changes and shouldn't be hard to our clients, so it's probably not worth the effort for the extra layer.
The only other notable bit is that config.tags = … within the Raven.configure block was a nice and obvious way to set default tags for all events.
Do you use the same DSN in multiple projects? Why do you need default tags that are the same in all events?
There was a problem hiding this comment.
Following #1101 (comment) we still use an enriched Exception object with a method specific for Sentry event's tags like with sentry_context and we use it like this
config.before_send = lambda do |event, hint|
exception = hint[:exception]
if exception.respond_to?(:sentry_tags_from_issue)
event.tags = exception.sentry_tags_from_issue
end
event
end| elsif exception.respond_to?(:sentry_context) | ||
| exception.sentry_context |
There was a problem hiding this comment.
Note how #sentry_context used to be able to provide every aspect of event context, not just extra.
| } } | ||
| { | ||
| foo: "bar" | ||
| } |
After #1086, all the APIs use a temporary Scope for assigning custom attributes to the Event. So Event's options aren't really used anymore. And this PR just removes the related code.