Skip to content

Allow backtrace cleaner to be configured#453

Merged
adrianna-chang-shopify merged 1 commit intomainfrom
configure-backtrace-cleaner
Jul 19, 2021
Merged

Allow backtrace cleaner to be configured#453
adrianna-chang-shopify merged 1 commit intomainfrom
configure-backtrace-cleaner

Conversation

@adrianna-chang-shopify
Copy link
Contributor

Rather than defaulting to Rails.backtrace_cleaner for cleaning the backtrace before persisting it in Run#persist_error, it would be nice if this could be configured. In Shopify/shopify, I need to add some silencers to remove lines related to our job infra / instrumentation from the backtrace, so I'd like to specify a new backtrace cleaner to be used by the gem for cleaning up Task backtraces.

I think it makes sense to expose this as a config option -- users might have job-related lines that they want to include in traces as part of their default Rails.backtrace_cleaner, but want to exclude from traces the gem shows.

@adrianna-chang-shopify adrianna-chang-shopify force-pushed the configure-backtrace-cleaner branch from f4f5efc to fdb5fdd Compare July 19, 2021 18:02
Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

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

You'll want to set the value in rails config and then add defaults in an initializer block. See https://github.com/Shopify/app_profiler/blob/78cb8b579a2bbf3034c6941f9adb865f5ef3d283/lib/app_profiler/railtie.rb#L10 (for example).

@adrianna-chang-shopify adrianna-chang-shopify force-pushed the configure-backtrace-cleaner branch from fdb5fdd to 59eb202 Compare July 19, 2021 18:12
@adrianna-chang-shopify
Copy link
Contributor Author

Thanks @gmcgibbon, I was hoping there was a better way to do that! ❤️

end

initializer "maintenance_tasks.configs" do
MaintenanceTasks.backtrace_cleaner = Rails.backtrace_cleaner
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
MaintenanceTasks.backtrace_cleaner = Rails.backtrace_cleaner
MaintenanceTasks.backtrace_cleaner ||= Rails.backtrace_cleaner

You could move configuration to a rails config option (eg. Rails.application.config.maintenance_tasks.*) in the engine, but this should work fine. Given there's already config options that don't follow this pattern, I think it is fine as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC we went with this approach because the Rails engine docs identified using mattr_accessors as a valid pattern for setting config options on engines, but I do think that using Rails config options would be more appropriate. Maybe this is something we can revisit -- although the process to deprecate this and move people towards using config options is maybe more hassle than it's worth.

@adrianna-chang-shopify adrianna-chang-shopify merged commit aa329c7 into main Jul 19, 2021
@adrianna-chang-shopify adrianna-chang-shopify deleted the configure-backtrace-cleaner branch July 19, 2021 18:32
lawrencewong pushed a commit to lawrencewong/maintenance_tasks that referenced this pull request Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants