es query rule - get time field from data view instead of rule #182883
Conversation
|
/ci |
mattkime
added
Team:DataDiscovery
|
/ci |
mattkime
changed the title
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
| @@ -120,7 +120,7 @@ export function updateSearchSource( | |||
|
|
|||
| searchSource.setField('size', isGroupAgg ? 0 : params.size); | |||
|
|
|||
| const field = index.fields.find((f) => f.name === timeFieldName); | |||
| const field = index.getTimeField(); | |||
There was a problem hiding this comment.
Maybe it's a right direction but I think we should be careful here. If for example user temporary changes the time field in the data view just to have a different view in Discover, I don't think it should affect alerts right away. It seems to me expected that the alerts logic uses a time field which was saved in its meta data during the rule creation instead of a current data view time field.
Were you able to reproduce the reported error TypeError: Cannot read properties of undefined (reading 'type')? I could not do that locally yet and wonder why it might result in undefined and where.
There was a problem hiding this comment.
I didn't reproduce but I think the error is because of a missing time field when a time filter is created
cannot read properties of undefined (reading 'type') - TypeError: Cannot read properties of undefined (reading 'type')\n at /usr/share/kibana/node_modules/@kbn/es-query/src/filters/build_filters/range_filter.js
How can a time field vanish?
- the time field name changed to a name that's not available in the actual field list, if I just change a time field to a different field, it should not fail
- the index pattern of a data view changed to a pattern that no longer contains this field
There was a problem hiding this comment.
I was able to reproduce the issue with situation 2 that @kertal describes:
- Create a data view with a certain time field
- Create a rule based on that data view
- Go and modify the data view to point at another index that doesn’t contain the previous time field
- The rule will fail to run with the expected error
Scenario 1 might be possible as well, although I believe we don't allow setting the data view time field to a field that isn't in one of its targeted indices, so it seems less likely. Maybe if modifying the data view via the API this could happen since I'm not sure if it's checked on the back end or not.
If for example user temporary changes the time field in the data view just to have a different view in Discover, I don't think it should affect alerts right away.
So we discussed this scenario in Slack as well -- if a user is depending on the current behaviour, even if it's buggy. I think we need to make a decision about which behaviour we consider "correct":
- Should alerts always try to first use the time field saved when the rule was created and only fall back to the current data view time field if the old one wasn't found?
- Or should modifying the data view time field also impact its associated alerts?
My opinion is that the second option is correct -- modifying a data view time field should modify the time field used by its alerts. IMO the other behaviour is too implicit and poorly defined, plus we notify users in the UI when making changes to a data view that it will affect functionality that depends on that data view and could break things. This should serve as a warning that maybe instead of a temporary change, they should create a separate or ad hoc data view with the same index pattern for their needs.
With the current behaviour, it's not clear how a user can update their rule to use the current time field if that's what they wanted to do. Possibly opening the rule and resaving it with the same data view would do the trick (haven't validated this yet), but that would be very implicit and it's not communicated to users anywhere. And even if it does work that way, they would have to do that for each rule that relies on that data view after editing it. So to me, having the time field always come from the data view directly makes the most sense.
That said, I'm open to going with the first approach if others feel there's a big enough risk to changing the behaviour. We at least know that approach would result in no change to functionality, even if it feels buggy to me.
There was a problem hiding this comment.
Yeah, I don't have a strong opinion on how it should work.
Should alerts always try to first use the time field saved when the rule was created and only fall back to the current data view time field if the old one wasn't found?
This might become confusing too and it does not cover a case when the data view does not have a time field any more. The alert would still fail with the same undefined error.
Or should modifying the data view time field also impact its associated alerts?
To make it clearer for future debugging/maintenance, we would need to additionally stop saving a time field in params in this PR. undefined can still happen when the data view does not have a time field any more.
ad hoc data view with the same index pattern for their needs
This can be a third option to consider but for our needs :) When user creates an alert for a data view, we save its ad-hoc copy inside rule params. This way changes to the original data view would not affect alerts. There is another problem here is that URLs for ad-hoc data views tend to be longer.
There was a problem hiding this comment.
This might become confusing too and it does not cover a case when the data view does not have a time field any more.
I agree about this being confusing, but I think it's the only one that addresses the current errors without changing existing behaviour (apart from new fallback behaviour) so I included it.
To make it clearer for future debugging/maintenance, we would need to additionally stop saving a time field in params in this PR.
I suggested this originally but the timeField param is apparently required for the rule type since it's the same type used for DSL and ES|QL which require the time field to be specified. Without creating an entirely separate rule type, the best we may be able to do is stub the param with some placeholder to indicate it isn't used.
I agree about your comments around undefined when updating the data view to have no time field, but I think this would be a legitimate error scenario that would be difficult to avoid. I think we could only address the situation where the updated data view still has a time field (unless we clone it for the alert like you mention below) while providing a clearer error message for when the data view is updated to have no time field, e.g. Rule {ruleName} could not execute because the target data view with ID {dataViewId} no longer contains a time field.
When user creates an alert for a data view, we save its ad-hoc copy inside rule params. This way changes to the original data view would not affect alerts.
This would solve it for sure, but is it ideal for users? Do they want a snapshot of the data view instead of a reference to it? They'd be left to update each individual rule that depends on that data view if they want to sync its changes, but maybe that's OK. One thing we'd need to do in this case is communicate clearly to users when they create a rule that it will use a snapshot of the data view instead of a reference, as well as providing a flow to allow syncing the snapshot with the latest data view changes when necessary.
There was a problem hiding this comment.
This might become confusing too and it does not cover a case when the data view does not have a time field any more. The alert would still fail with the same undefined error.
We should probably throw an error.
To make it clearer for future debugging/maintenance, we would need to additionally stop saving a time field in params in this PR.
I think that makes sense. I was aiming for a quick fix with this PR. Perhaps that could be addressed in a follow up PR.
Generally speaking, the conversation about whether this is the right fix is a conversation that comes up occasionally but it always boils down to "What if users don't understand the implications of their changes?" Yes, that can be a problem, that's what ad hoc data views are for, limiting the impact of changes to a saved object. Obviously implementing this could be a bigger effort.
That aside, I wonder how often we get feedback from users complaining about problems understanding saved object changes.
Ensure we're using the data view time field for the `sort` param too
|
/ci |
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @mattkime |
…c#182883) ## Summary Previously it was possible to create a rule with a data view and change the data view but the previous time field would still be referenced. Now the time field is always pulled from the current data view. Closes elastic#182879 #### Release note Fixed issue where an ES query rule could be created with a data view, then the data view is changed but there's still a reference to the previous data view's timestamp field. Now the timestamp field is always taken from the currently configured data view. --------- Co-authored-by: Davis McPhee <[email protected]> (cherry picked from commit bc103c7)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…182883) (#183001) # Backport This will backport the following commits from `main` to `8.14`: - [es query rule - get time field from data view instead of rule (#182883)](#182883) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Matthew Kime","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-05-08T22:00:24Z","message":"es query rule - get time field from data view instead of rule (#182883)\n\n## Summary\r\n\r\nPreviously it was possible to create a rule with a data view and change\r\nthe data view but the previous time field would still be referenced. Now\r\nthe time field is always pulled from the current data view.\r\n\r\n\r\nCloses https://github.com/elastic/kibana/issues/182879\r\n\r\n#### Release note\r\n\r\nFixed issue where an ES query rule could be created with a data view,\r\nthen the data view is changed but there's still a reference to the\r\nprevious data view's timestamp field. Now the timestamp field is always\r\ntaken from the currently configured data view.\r\n\r\n---------\r\n\r\nCo-authored-by: Davis McPhee <[email protected]>","sha":"bc103c7016245901a04fc4921c1b213a4fbe2695","branchLabelMapping":{"^v8.15.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:DataDiscovery","backport:prev-minor","v8.15.0"],"title":"es query rule - get time field from data view instead of rule","number":182883,"url":"https://github.com/elastic/kibana/pull/182883","mergeCommit":{"message":"es query rule - get time field from data view instead of rule (#182883)\n\n## Summary\r\n\r\nPreviously it was possible to create a rule with a data view and change\r\nthe data view but the previous time field would still be referenced. Now\r\nthe time field is always pulled from the current data view.\r\n\r\n\r\nCloses https://github.com/elastic/kibana/issues/182879\r\n\r\n#### Release note\r\n\r\nFixed issue where an ES query rule could be created with a data view,\r\nthen the data view is changed but there's still a reference to the\r\nprevious data view's timestamp field. Now the timestamp field is always\r\ntaken from the currently configured data view.\r\n\r\n---------\r\n\r\nCo-authored-by: Davis McPhee <[email protected]>","sha":"bc103c7016245901a04fc4921c1b213a4fbe2695"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.15.0","branchLabelMappingKey":"^v8.15.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/182883","number":182883,"mergeCommit":{"message":"es query rule - get time field from data view instead of rule (#182883)\n\n## Summary\r\n\r\nPreviously it was possible to create a rule with a data view and change\r\nthe data view but the previous time field would still be referenced. Now\r\nthe time field is always pulled from the current data view.\r\n\r\n\r\nCloses https://github.com/elastic/kibana/issues/182879\r\n\r\n#### Release note\r\n\r\nFixed issue where an ES query rule could be created with a data view,\r\nthen the data view is changed but there's still a reference to the\r\nprevious data view's timestamp field. Now the timestamp field is always\r\ntaken from the currently configured data view.\r\n\r\n---------\r\n\r\nCo-authored-by: Davis McPhee <[email protected]>","sha":"bc103c7016245901a04fc4921c1b213a4fbe2695"}}]}] BACKPORT--> Co-authored-by: Matthew Kime <[email protected]>
Summary
Previously it was possible to create a rule with a data view and change the data view but the previous time field would still be referenced. Now the time field is always pulled from the current data view.
Closes #182879
Release note
Fixed issue where an ES query rule could be created with a data view, then the data view is changed but there's still a reference to the previous data view's timestamp field. Now the timestamp field is always taken from the currently configured data view.