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

feat: Add initial support for hooks #256

Merged
merged 3 commits into from
Mar 21, 2024
Merged

Conversation

keelerm84
Copy link
Member

No description provided.

@keelerm84 keelerm84 requested a review from a team March 20, 2024 16:26
Copy link

This pull request has been linked to Shortcut Story #236778: Add hook support to ruby.

#
# @return [Metadata]
#
def metadata
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking if it would be possible to force implementation of metadata. But it would probably just end up like the JS implementation. Where ultimately we end up falling back anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could raise an exception. That would force people to implement it. Want me to make that change?

return
end

@hooks.push(hook)
Copy link
Member

Choose a reason for hiding this comment

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

I don't remember, does ruby have threading concerns?

Copy link
Member Author

Choose a reason for hiding this comment

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

Like your node implementation, I make a copy of the hooks before I begin processing, so I don't think there is a thread issue here.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I wasn't sure if you actually had to lock the collection. In node there aren't threads, so it doesn't have to worry about any memory synchronization stuff.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually in thinking about it more, I do need to do some locking. This would work fine in MRI, but jRuby would probably have some sync issues since it doesn't use a GIL.

Updated to use the concurrent package we have used elsewhere.

# Execute the :before_evaluation stage of the evaluation series.
#
# This method will return the results of each hook, indexed into an array in the same order as the hooks. If a hook
# raised an uncaught exception, the value will be nil.
Copy link
Member

Choose a reason for hiding this comment

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

Would this nil propagate to the after?

Copy link
Member Author

@keelerm84 keelerm84 Mar 20, 2024

Choose a reason for hiding this comment

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

Yes. The results of this method get returned as an array. We then use that array in the after_evaluation .zip call to pair it with the appropriate hook.

@keelerm84 keelerm84 merged commit 7af3502 into feat/hooks Mar 21, 2024
1 check passed
@keelerm84 keelerm84 deleted the mk/sc-236778/hooks branch March 21, 2024 14:46
keelerm84 added a commit that referenced this pull request Apr 3, 2024
keelerm84 added a commit that referenced this pull request Apr 4, 2024
keelerm84 added a commit that referenced this pull request Apr 5, 2024
keelerm84 added a commit that referenced this pull request Apr 5, 2024
🤖 I have created a release *beep* *boop*
---

This release introduces a Hooks API. Hooks are collections of
user-defined callbacks that are executed by the SDK at various points of
interest. You can use them to augment the SDK with metrics or tracing.

##
[8.4.0](8.3.1...8.4.0)
(2024-04-05)


### Features

* Add initial support for hooks
([#256](#256))
([3cf16eb](3cf16eb))


### Bug Fixes

* Adjust migration variation and hook interaction
([#264](#264))
([8067af6](8067af6))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Matthew Keeler <[email protected]>
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.

2 participants