Skip to content

ActiveSupport::Notifications instrumentation of aws sdk client calls #37

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

Merged
merged 6 commits into from
Nov 11, 2020

Conversation

alextwoods
Copy link
Contributor

@alextwoods alextwoods commented Nov 7, 2020

Fixes: #14

Description of changes:
Support ActiveSupport::Notification instrumentation of all AWS SDK client operations.

This is currently not enabled by default and requires calling Aws::Rails.instrument_sdk_operations (eg, can be done in config/application.rb or in environment configs).

Instrumentation adds ~0.2ms per call (which is an ~20% increase outside of network calls).

Note: I had to inspect all loaded Aws modules for service modules and add the handler to each one - adding to the seahorse base client does not work at this stage.

Open Questions:

  1. Should this be enabled/disabled by config? And if so, should we allow configuration of which clients are instrumented. Answer: Removed config. Instrumentation is added by calling Aws::Rails.instrument_sdk_operations (See above explanation)
  2. What should the name of the event be? Answer: <operation>.<serviceId>.aws (Note, this naming matches the patterns used in rails).
  3. Is there a better way to do this than adding a plugin/handler? Answer: No, this is the best approach here.

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

@alextwoods alextwoods marked this pull request as ready for review November 9, 2020 21:47
README.md Outdated
Comment on lines 128 to 131
Example usage in `config/capplication.rb`
```ruby
Aws::Rails.instrument_sdk_operations
```
Copy link
Contributor

Choose a reason for hiding this comment

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

let's prefer to make an initializer here

@@ -1,10 +1,13 @@
# frozen_string_literal: true

require_relative 'aws/rails/mailer'
require_relative 'aws/rails/notifications_instrument_plugin'
Copy link
Contributor

Choose a reason for hiding this comment

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

you're going to need to rebase this against my change once merged, or vice versa, up to you.

@alextwoods alextwoods merged commit f420a1a into master Nov 11, 2020
@alextwoods alextwoods deleted the notification branch November 11, 2020 00:01
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.

Feature Request: ActiveSupport Notifications
2 participants