Skip to content

Remove Event options#1101

Merged
st0012 merged 3 commits intomasterfrom
refactor
Nov 7, 2020
Merged

Remove Event options#1101
st0012 merged 3 commits intomasterfrom
refactor

Conversation

@st0012
Copy link
Copy Markdown
Contributor

@st0012 st0012 commented Nov 7, 2020

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.

Right now the Event is updated via the Scope instead of
direct options. So those options aren't actually used.
@st0012 st0012 added this to the 4.0.0 milestone Nov 7, 2020
@st0012 st0012 self-assigned this Nov 7, 2020
@st0012 st0012 merged commit 05419ac into master Nov 7, 2020
@st0012 st0012 deleted the refactor branch November 7, 2020 07:44
def add_exception_interface(exc)
if exc.respond_to?(:sentry_context)
@extra.merge!(exc.sentry_context)
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
end

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
  end

Just curious, if there're other issues that blocked you from migrating to sentry-ruby?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines -61 to -62
elsif exception.respond_to?(:sentry_context)
exception.sentry_context
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note how #sentry_context used to be able to provide every aspect of event context, not just extra.

} }
{
foo: "bar"
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Illustrates the breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants