Scripting: Add watcher script contexts#34059
Conversation
This commit removes the use of ExecutableScript from watcher in favor of custom script contexts for both watcher condition scripts and transform scripts.
rjernst
added
:Core/Infra/Scripting
|
Pinging @elastic/es-core-infra |
hub-cap
left a comment
hub-cap
left a comment
There was a problem hiding this comment.
These changes look fine to me, but i wouldnt mind @spinscale also taking a look. I dont know much about Scripts in watcher.
| return model; | ||
| } | ||
|
|
||
| public static Map<String, Object> createCtx(WatchExecutionContext ctx, Payload payload) { |
There was a problem hiding this comment.
do we still need this? should it be @deprecated?
There was a problem hiding this comment.
Not sure what you mean. This was extracted from the previous method to allow only getting the actual ctx map, but not putting it into an outer params map (the new contexts need this so they can put it into the wrapped ParameterMap for deprecation warnings).
There was a problem hiding this comment.
yea i just mean if we dont want coders (me and @spinscale) accidentally using the createCtx method, u could private it, or if other things are still using it, mark it @deprecated in favor of using the other method createCtxParamsMap
There was a problem hiding this comment.
Both are used for now. I will add a comment to try to make it clearer which should be used when.
There was a problem hiding this comment.
could you have just a second createCtxParamsMap method that takes another map as parameter? Which is an empty hash map by default used to fill and the parmaters in WatcherConditionScript?
There was a problem hiding this comment.
I think having 2 methods with the same name would be more confusing. Do the java docs I added not make sense?
This commit adds the ability to plug in compilation of custom contexts in mock script engine. This is needed for testing plugins which add custom contexts like watcher.
spinscale
left a comment
spinscale
left a comment
There was a problem hiding this comment.
My Script Context skills are not the best, from a watcher perspective this looks good to me though.
Left one question regarding adding tests for the deprecation.
| * A script to determine whether a watch should be run. | ||
| */ | ||
| public abstract class WatcherConditionScript { | ||
| public static final String[] PARAMETERS = {}; |
There was a problem hiding this comment.
This constant must exist for all script classes, even if the execute method takes zero arguments.
| @@ -31,7 +27,7 @@ public final class ScriptCondition implements ExecutableCondition { | |||
|
|
|||
| private final ScriptService scriptService; | |||
| Map<String, String> deprecations = new HashMap<>(); | ||
| deprecations.put( | ||
| "ctx", | ||
| "Accessing variable [ctx] via [params.ctx] from within a watcher_transform script " + |
There was a problem hiding this comment.
is it worth testing this somewhere or did I miss the test? Same applies for the WatcherConditionScript
This commit removes the use of ExecutableScript from watcher in favor of custom script contexts for both watcher condition scripts and transform scripts.
This commit removes the use of ExecutableScript from watcher in favor of custom script contexts for both watcher condition scripts and transform scripts.
This commit removes the use of ExecutableScript from watcher in favor of custom script contexts for both watcher condition scripts and transform scripts.
When the script contexts were created in 6, the use of params.ctx was deprecated. This commit cleans up that code and ensures that params.ctx is null in both watcher script contexts. Relates: elastic#34059
When the script contexts were created in 6, the use of params.ctx was deprecated. This commit cleans up that code and ensures that params.ctx is null in both watcher script contexts. Relates: #34059
|
For reference, this PR prevented me to use the same Painless script for both the watch condition and transformation because the condition now enforces the return type to be a boolean. The Elastic support was kind enough to work out a solution with me which still allows to only use one script for both: ---
# yamllint disable rule:line-length rule:comments-indentation
trigger:
schedule:
hourly:
minute: 32
input:
search:
timeout: '5m'
request:
indices:
- '*-*'
- '-.*'
body:
size: 0
query:
bool:
must:
- exists:
field: '@timestamp'
condition:
script:
inline: |
ctx.payload.transformed= [ '_doc': [ ] ];
return false;
transform:
script:
inline: 'return ctx.payload.transformed;'
actions:
log:
logging:
level: info
text: 'Watch payload after transform: {{ctx.payload}}' |
6 participants
This commit removes the use of ExecutableScript from watcher in favor of
custom script contexts for both watcher condition scripts and transform
scripts.