Add plugins audit log #327
Conversation
|
Also renamed to |
src/worker/plugins/run.ts
Outdated
| if (!returnedEvent.properties) { | ||
| returnedEvent.properties = {} | ||
| } | ||
| if (pluginsRan.length > 0) { | ||
| returnedEvent.properties['$plugins_ran_successfully'] = pluginsRan | ||
| } | ||
| if (pluginsFailed.length > 0) { | ||
| returnedEvent.properties['$plugins_failed'] = pluginsFailed | ||
| } |
There was a problem hiding this comment.
Some code feedback then :).
- There's no need to run
if (!returnedEvent.properties) { returnedEvent.properties = {} }if we're not going to change anything (when lengths are 0 for both arrays) - I think it's better that either both keys exist or none do, otherwise you'll end up second guessing yourself if the $plugins_failed array is just not there (instead of being empty, which tells you directly that nothing failed)
You could do something like:
if (pluginsRan.length > 0 || pluginsFailed.length > 0) {
event.properties = {
...event.properties,
'$plugins_ran_successfully': pluginsRan,
'$plugins_failed': pluginsFailed,
}
}
src/worker/plugins/run.ts
Outdated
| if (event) { | ||
| if (!event.properties) { | ||
| event.properties = {} | ||
| } | ||
| event.properties['$plugins_ran_successfully'] = pluginsRan | ||
| event.properties['$plugins_failed'] = pluginsFailed | ||
| } |
There was a problem hiding this comment.
Same here that we should only add these props if any plugins actually ran.
src/worker/plugins/run.ts
Outdated
| @@ -66,6 +81,16 @@ export async function runPluginsOnBatch(server: PluginsServer, batch: PluginEven | |||
| } | |||
| } | |||
|
|
|||
| for (const event of returnedEvents) { | |||
| if ((event && pluginsRan.length > 0) || pluginsFailed.length > 0) { | |||
There was a problem hiding this comment.
this if is looking funny... :)
There was a problem hiding this comment.
huh formatter messed up there
There was a problem hiding this comment.
Did it now? :P
Or did you just forget the () and prettier just made the default operator precedence apparent? :)
Always when mixing && and || put as many brackets around the different parts of the query as possible to avoid any errors...
|
A flaky test that's not related to this PR is failing. So merging and will fix that later. |
|
Ah, no, I can't :D
Will re-run and hope for the best. |
* Add audit log of plugins ran and failed * Add plugins audit log * updates * update tests * address feedback * update tests * override formatter * rename $plugins_ran_successfully to $plugins_succeeded Co-authored-by: Marius Andra <[email protected]>
Changes
Tackles #269
I guess
$plugins_failedwill need to evolve once we have retries, but we don't yet, so...Checklist