Automatically ignore Rails' health-check

[unflxw]

Suggested by a customer: https://app.intercom.com/a/inbox/yzor8gyw/inbox/shared/unassigned/conversation/16410700366724#part_id=comment-16410700366724-21909575460

(leaving this as a draft to not forget; will make a proper PR later)

Adds Rails::HealthController#show to the default ignore_actions
value.

Modified some existing tests that used ignore_actions as an array
config option that defaulted to empty, so that they use ignore_errors
instead.

See https://github.com/appsignal/diagnose_tests/compare/add-ignore-actions-ruby-default-value for diagnose tests changes.

[backlog-helper]

✔️ All good!

New issue guide | Backlog management | Rules | Feedback

[unflxw]

Wondering how to move forward with this. The idea of ignoring this by default makes sense to me.

But adding it into the config option means that, when customers override the config option, they'll override this as well, forcing them to add it back manually as part of the override. However, that's also how the REQUEST_HEADERS config option works, so maybe it's fine.

We could make the config option merge with the default list instead of replacing it, but then that's equivalent to it being hardcoded -- there's no way customers can delete this entry from the list. Which is fine, probably? I see no use case for wanting calls to this endpoint to be instrumented.

EDIT: Decided to go with the first option for now, open to changing it though. Will wait to move forward until @tombruijn is back.

[tombruijn]

Wondering how to move forward with this. The idea of ignoring this by default makes sense to me.

But adding it into the config option means that, when customers override the config option, they'll override this as well, forcing them to add it back manually as part of the override. However, that's also how the REQUEST_HEADERS config option works, so maybe it's fine.

We could make the config option merge with the default list instead of replacing it, but then that's equivalent to it being hardcoded -- there's no way customers can delete this entry from the list. Which is fine, probably? I see no use case for wanting calls to this endpoint to be instrumented.

EDIT: Decided to go with the first option for now, open to changing it though. Will wait to move forward until @tombruijn is back.

This is a limitation for the YAML file mostly. In the Appsignal.configure helper, which I want to make the recommended config method, apps can merge their config and remove defaults if they want.

Appsignal.configure do |config|
  # Overwrite defaults with a new list of ignored actions
  config.ignore_actions = ["MyController#action"]
  # Delete specific defaults and keep other gem defaults
  config.ignore_actions.delete("Rails::HealthController#show")
  # Merge app config with gem defaults
  config.ignore_actions << "MyController#action"
end

For the environment variables I hope this is less of an issue and we can wait until someone presents us a case where it needs to be done using environment variables, and cannot be somehow done with the Appsignal.configure helper.

[tombruijn]

I think it would be better to add these to the Rails loader defaults, like the other Rails specific config. It will be part of the config of apps using both Rails and another framework.

appsignal-ruby/lib/appsignal/integrations/railtie.rb

Line 43 in 6201f5d

Appsignal::Config.add_loader_defaults(

[unflxw]

Feedback needs to be addressed and approved before merging.

[tombruijn]

I can pick this up while Noemi is away.

[tombruijn]

Created a new PR with a different implementation here: #1321