-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
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.
Pinging @elastic/es-core-infra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need this? should it be @deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This constant must exist for all script classes, even if the execute method takes zero arguments.
@@ -31,7 +27,7 @@ | |||
|
|||
private final ScriptService scriptService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused now?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it worth testing this somewhere or did I miss the test? Same applies for the WatcherConditionScript
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added tests in e1e0490
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}}' |
This commit removes the use of ExecutableScript from watcher in favor of
custom script contexts for both watcher condition scripts and transform
scripts.