Cross-compatible hook/middleware + "plugins" which are allowed to be hook and middleware#1284
Cross-compatible hook/middleware + "plugins" which are allowed to be hook and middleware#1284brandur wants to merge 4 commits into
Conversation
bgentry
left a comment
bgentry
left a comment
There was a problem hiding this comment.
Needs changelog + homepage docs, but lgtm! 🚀
…hook and middleware This one's largely aimed at extending a few parts of `otelriver` to be able to emit some additional useful metrics like time that it takes to lock jobs, number of jobs locked per batch, or any other arbitrary metrics we want to emit down the road. I previously had something similar back in #1203, but here we extract an isolated piece of it. This change does two things: * If either a hook sent to `Config.Hooks` implements a middleware or a middleware sent to `Config.Middleware` implements a hook, activate its alternate side as well. * Establish a new `Config.Plugins` that acts as a more generalized place where a hook/middleware can go. We define a plugin as this type: type Plugin interface { Hook Middleware } The reason for the first point is better backward compatibility. Notably, if I add a hook to `otelriver.Middleware`, I want it to be able to still work even if the user doesn't explicitly movie it from `Config.Middleware` to `Config.Plugins`.
Make hooks and middleware opt into `Plugin` through their default embedders and function helpers. Keep `Config.Plugins` as the unified registration path.
Merge the split lookup packages into one precomputed plugin lookup. Build the kind buckets up front and use the unified path throughout client startup and job execution.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e83736133
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for _, hook := range hooks { | ||
| if plugin, ok := hook.(rivertype.Plugin); ok { | ||
| normalizedPlugins = append(normalizedPlugins, plugin) | ||
| } | ||
| } | ||
| for _, middleware := range middlewares { | ||
| if plugin, ok := middleware.(rivertype.Plugin); ok { | ||
| normalizedPlugins = append(normalizedPlugins, plugin) |
There was a problem hiding this comment.
NormalizePlugins now only carries registrations forward when they also satisfy rivertype.Plugin, but the public Config.Hooks, Config.Middleware, and JobArgsWithHooks APIs still accept plain rivertype.Hook/rivertype.Middleware values. In applications with custom hooks or middleware that manually implement IsHook/IsMiddleware (or embed both defaults without adding an unambiguous IsPlugin), those entries are silently dropped from every lookup, so their insert/work behavior stops running after this change; the original hook/middleware side should still be preserved even when the value is not a plugin.
Useful? React with 👍 / 👎.
| JobRow: job, | ||
| MiddlewareLookupGlobal: middlewarelookup.NewMiddlewareLookup(append(rivermiddleware.DefaultMiddleware(), w.config.Middleware...)), | ||
| PluginLookupByJob: pluginlookup.NewJobPluginLookup(), | ||
| PluginLookupGlobal: pluginlookup.NewPluginLookup(pluginlookup.NormalizePlugins(effectiveHooks, append(rivermiddleware.DefaultMiddleware(), effectiveMiddleware...), nil)), |
There was a problem hiding this comment.
In rivertest.Worker, effectiveHooks and effectiveMiddleware already expand cross-compatible registrations and plugins into both slices, but this line then normalizes both slices as independent plugin sources. When a test config uses a Config.Hooks item that also implements middleware, a Config.Middleware item that also implements hooks, or a Config.Plugins item implementing both, the same value is added twice to the plugin lookup and its hook/middleware runs twice under WorkJob, diverging from real Client behavior.
Useful? React with 👍 / 👎.
2 participants
This one's largely aimed at extending a few parts of
otelriverto beable to emit some additional useful metrics like time that it takes to
lock jobs, number of jobs locked per batch, or any other arbitrary
metrics we want to emit down the road. I previously had something
similar back in #1203, but here we extract an isolated piece of it.
This change does two things:
If either a hook sent to
Config.Hooksimplements a middleware or amiddleware sent to
Config.Middlewareimplements a hook, activate itsalternate side as well.
Establish a new
Config.Pluginsthat acts as a more generalized placewhere a hook/middleware can go. We define a plugin as this type:
The reason for the first point is better backward compatibility.
Notably, if I add a hook to
otelriver.Middleware, I want it to be ableto still work even if the user doesn't explicitly move it from
Config.MiddlewaretoConfig.Plugins.