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

Add the action instance to the request environment #446

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

tombruijn
Copy link
Contributor

I created a new topic on the Hanami Discourse about improving the AppSignal APM instrumentation.

To group requests, we would like to know what action it took place in. This information is currently not available in the request environment.

For us, the easiest would be to access the action instance. That gives us the class information and a way to access the params_class to fetch all the parameters of the request: query params and the body payload.

This change adds the action instance on a new request environment key hanami.action_instance.

I had to update one test to not fail on the action instance being returned, which is not important for that spec I think.

Closes #445


Let me know what you think!

tombruijn and others added 2 commits November 3, 2024 23:15
I created a [new topic] on the Hanami Discourse about improving the
AppSignal APM instrumentation.

To group requests, we would like to know what action it took place in.
This information is currently not available in the request environment.

For us, the easiest would be to access the action instance. That gives
us the class information and a way to access the `params_class` to fetch
all the parameters of the request: query params and the body payload.

This change adds the action instance on a new request environment key
`hanami.action_instance`.

[new topic]: https://discourse.hanamirb.org/t/questions-for-improving-the-appsignal-apm-integration-with-hanami-2/989/3

I had to update one test to not fail on the action instance being
returned, which is not important for that spec I think.

Closes hanami#445
This way we don’t interfere with the expected contents of params when actions are unit tested manually and params are passed as a plain hash.
@timriley
Copy link
Member

timriley commented Nov 3, 2024

Hi @tombruijn!! Thanks for your patience with this one.

The Hanami 2.2 release is happening in a couple of days, and I've found myself with enough time to look at getting this PR in.

I'd done some changes to the params handling and tests in this repo, so I've rebased over the main branch and force pushed to this PR's branch, I hope you don't mind.

I've also made one other change (this commit): I delayed setting env[ACTION_INSTANCE] until the end of Hanami::Action#call. This way when users are doing simplistic unit tests of actions and passing params as a plain hash (an affordance we allow to simplify testing), they won't have to worry about a confusing "hanami.action_instance" key being a part of their params every time.

I've looked at the way you inspect the env for Rails' "action_controller.instance", and it seems like this is done after the controller/Rack app has been called, so it would seem to be that in our case you wouldn't be sensitive to when we set the env["hanami.action_instance"], so long as it's there by the time the action call is completed. Is that right?

I'm sorry for the short notice, but if you can confirm this in the next day or two, I'll be able to include this in the Hanami 2.2 release, which would be wonderful 😄

Thanks!

@timriley
Copy link
Member

timriley commented Nov 4, 2024

You know what? I might just go ahead and merge this. I suspect it'll still do the trick, and if we need to adjust it, that's easy to do in a patch release :)

@timriley timriley merged commit 5d8d610 into hanami:main Nov 4, 2024
4 checks passed
@timriley timriley self-assigned this Nov 4, 2024
@tombruijn tombruijn deleted the request-env-action-instance branch November 4, 2024 09:46
@tombruijn
Copy link
Contributor Author

I've looked at the way you inspect the env for Rails' "action_controller.instance", and it seems like this is done after the controller/Rack app has been called, so it would seem to be that in our case you wouldn't be sensitive to when we set the env["hanami.action_instance"], so long as it's there by the time the action call is completed. Is that right?

Hi @timriley! That seems good 👍 Most of our instrumentations set things like the action name after the request is done already. We've found a lot of libraries don't know things like the action name, parsed params, route name, etc. beforehand, so our gem can handle this order of data being available.

Thanks for updating it and merging it! We'll update our Ruby gem to take advantage of it so we have to do no/less monkeypatching for Hanami 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add action name to Rack environment
2 participants