Skip to content

Instrument ActiveStorage analyzers #42939

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 2 commits into from
Aug 28, 2021

Conversation

shouichi
Copy link
Contributor

@shouichi shouichi commented Aug 4, 2021

Summary

This PR adds instrumentations to ActiveStorage analyzers so that the time of analysis can be tracked.

Close #42930.

Other Information

The following things should be agreed upon before merging.

  • Name of the instrumentation event
    • Currently analyzer.active_storage but should we include analyzer name? Changed to analyze.active_storage so that it is consistent with others.
  • Extra payload to attach (not sure if there any.)
    • Add analyzer name.

Also, we may want to add the new event to the active support instrumentation document.

Thanks.

@p8
Copy link
Member

p8 commented Aug 16, 2021

This looks great! Can you rebase and add a Changelog entry?

@p8
Copy link
Member

p8 commented Aug 16, 2021

Could you also add it to the guides? 🙏
https://edgeguides.rubyonrails.org/active_support_instrumentation.html#active-storage

@shouichi shouichi force-pushed the instrument-analyzers branch from 9e32e8b to 3fb656c Compare August 17, 2021 02:11
@rails-bot rails-bot bot added the docs label Aug 17, 2021
@shouichi shouichi force-pushed the instrument-analyzers branch from 3fb656c to f07c73e Compare August 17, 2021 02:14
@shouichi
Copy link
Contributor Author

shouichi commented Aug 17, 2021

Hi, @p8 thanks for the review!

Rebased and made the following changes.

  • Changed event name to analyze.active_storage so that it is consistent with others (e.g., transform.active_storage).
  • Added tests to check if a payload contains an analyzer name.
  • Added CHANGELOG
  • Added the new event to the active support instrumentation guide.

Copy link
Member

@p8 p8 left a comment

Choose a reason for hiding this comment

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

Could you also rebase on main to hopefully fix the unrelated spelling warnings?

@shouichi shouichi force-pushed the instrument-analyzers branch from f07c73e to 35d8890 Compare August 17, 2021 07:55
@shouichi shouichi requested a review from p8 August 17, 2021 07:56
@shouichi
Copy link
Contributor Author

Rebased 👍

@p8 p8 added the ready PRs ready to merge label Aug 17, 2021
@p8
Copy link
Member

p8 commented Aug 17, 2021

Thanks! The build failures seem unrelated :/

@p8 p8 removed the ready PRs ready to merge label Aug 17, 2021
Co-authored-by: Petrik de Heus <petrik@deheus.net>
@p8 p8 added the ready PRs ready to merge label Aug 17, 2021
@guilleiguaran guilleiguaran merged commit b1b8f8a into rails:main Aug 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instrument ActiveStorage analyzer
3 participants