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

Migrate from sentry-raven to sentry-ruby #8818

Merged
merged 9 commits into from
Jan 23, 2024
Merged

Conversation

JamieMagee
Copy link
Contributor

@JamieMagee JamieMagee commented Jan 17, 2024

This migrates from the deprecated sentry-raven gem to the replacement sentry-ruby gem. sentry-raven was deprecated in March 2021.

The changes mostly follow the Sentry migration guide. The largest changes were around event processors. Previously, sentry-raven allowed you to inherit from Raven::Processor and pass it to processors during configuration. In sentry-ruby that has been replaced with before_send which accepts a single lambda. I've recreated the same behaviour by using a reducer to apply processors consecutively. See the "scrubbing sensitive data" and "Filtering for Ruby" documentation for more information.

Once this is merged, it opens up the possibility of using the sentry-opentelemetry gem as well.

@deivid-rodriguez
Copy link
Contributor

Awesome, it was about time! See #7553 for previous art. This PR can supersede that one but maybe you find something useful in the code/comments there!

@JamieMagee JamieMagee force-pushed the jamiemagee/sentry-ruby branch 11 times, most recently from 9c2a457 to fe8e3f2 Compare January 18, 2024 19:51
@JamieMagee JamieMagee force-pushed the jamiemagee/sentry-ruby branch 2 times, most recently from ce2bfe8 to ca42235 Compare January 18, 2024 23:10
Comment on lines +18 to +22
if (exception = hint[:exception])
exception.raven_context.each do |key, value|
event.send("#{key}=", value)
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -39,7 +41,8 @@
swift
)}x

config.processors += [ExceptionSanitizer]
config.before_send = ->(event, hint) { Dependabot::Sentry::Processor.process_chain(event, hint) }
config.propagate_traces = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This prevents Sentry adding Sentry-Trace and Baggage headers by default to all outgoing requests. It's designed to make it easier to do distributed tracing between microservices or frontend/backend. As we don't have that architecture, and it currently breaks our smoke test cache, I've chosen to disable it by default.

If we want to get this distributed trace information, we'll need to ignore these headers in our updater proxy.

References:

Copy link
Member

Choose a reason for hiding this comment

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

I think disabling is the right call as Dependabot makes most calls to external endpoints that don't care about our tracing.

Comment on lines +106 to +116
tags: tags.merge({
"gh.dependabot_api.update_job.id": job&.id,
"gh.dependabot_api.update_config.package_manager": job&.package_manager,
"gh.repo.is_private": job&.repo_private?
}.compact),
extra: extra.merge({
dependency_name: dependency&.name,
dependency_group: dependency_group&.name
}.compact),
user: {
id: job&.repo_owner
Copy link
Contributor Author

Choose a reason for hiding this comment

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

# typed: strong
# frozen_string_literal: true

module Sentry
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tapioca gem sentry-ruby generates an empty file, so this is minimal handwritten types for our uses.

@@ -39,7 +41,8 @@
swift
)}x

config.processors += [ExceptionSanitizer]
config.before_send = ->(event, hint) { Dependabot::Sentry::Processor.process_chain(event, hint) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

config.processors has been removed in favour of a lambda before_send. I've replicated the behaviour by using a reducer to call processors in order.

I had to migrate raven_context to a custom processor as well, because raven_context is no longer attached to events by default.

@JamieMagee JamieMagee marked this pull request as ready for review January 18, 2024 23:27
@JamieMagee JamieMagee requested a review from a team as a code owner January 18, 2024 23:27
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Fantastic job! ❤️

@jeffwidman
Copy link
Member

jeffwidman commented Jan 23, 2024

Thanks @JamieMagee for tackling this, one less hanging TODO that I feel bad about not having the time to finish 😊 plus I was always annoyed that Dependabot was relying on an EOL'd library.

Reading through this PR was a lot of fun 🎉 because it answered several of my questions from #7553.

In case you didn't see this note from my PR:

Events now default to sending asynchronously using a thread pool whereas raven used to send them synchronously.

I suspect this change is harmless, but still, might be worth double-checking when this gets deployed, just in case. More details here, including how to preserve the old behavior through configuration if necessary.

@abdulapopoola abdulapopoola merged commit 623a5c3 into main Jan 23, 2024
119 checks passed
@abdulapopoola abdulapopoola deleted the jamiemagee/sentry-ruby branch January 23, 2024 17:59
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.

None yet

5 participants