-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[RAM] Window Maintenance Client/Saved Object and Mapping/REST APIs #153411
[RAM] Window Maintenance Client/Saved Object and Mapping/REST APIs #153411
Conversation
…-ref HEAD~1..HEAD --fix'
Pinging @elastic/response-ops (Team:ResponseOps) |
@elasticmachine merge upstream |
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.
LGTM!
startDate: string | null; | ||
endDate: string | null; |
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.
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.
eventStartDate
sounds good to me, I agree that start and end date could be confusing
@elasticmachine merge upstream |
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.
Looks good from our perspective! Thanks for making the SO namespace type change!
savedObjects.registerType({ | ||
name: MAINTENANCE_WINDOW_SAVED_OBJECT_TYPE, | ||
hidden: true, | ||
namespaceType: 'multiple-isolated', |
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.
👍
@elasticmachine merge upstream |
@elasticmachine merge upstream |
} | ||
|
||
// Returns the most recent/relevant event and the status for a maintenance window | ||
export const getMaintenanceWindowDateAndStatus = ({ |
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.
it looks like this function is not used any where, is that correct?
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.
It's being used in getMaintenanceWindowFromRaw
, basically we use this helper to decorate the maintenance window SO with the current status
, the event_start/end_dates
when returned by the API, kind of similar to getAlertFromRaw
}); | ||
|
||
if (!shouldRegenerateEvents({ maintenanceWindow: attributes, rRule, duration })) { | ||
events = mergeEvents({ oldEvents: attributes.events, newEvents: events }); |
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.
am I thinking about it to simplistic here? but why can we just trust the new events and delete the old events. I imagine that the new events will have duplicate.
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.
The main reason is to make sure we do not overwrite events that have been finished early. When we finish an event, we modify that event entry's end date to be now
, but if we edit the rule and just regenerate everything, it would overwrite that finished event and would go back to being running
.
The more robust way of doing this is to keep another array of modified events (I think google calendar does this). But obviously, it's more work, modifying 1 event works fine for now, we just need to be careful when we regenerate the event array.
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.
Like discussed, I will do a filter for modified events in the old event array, and replace the new event array with the modified old event.
import { SavedObjectsTypeMappingDefinition } from '@kbn/core/server'; | ||
import { rRuleMappingsField } from './rrule_mappings_field'; | ||
|
||
export const maintenanceWindowMappings: SavedObjectsTypeMappingDefinition = { |
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.
Let's go over together but I do not think that we need add this attributes to be indexed, only the one we are going to have sort/filter/search on it. Since we are going to do all this feature in memory of the UI, I do not think that we need anything. what do you think?
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 sounds good to me, I'm not sure how indexing works behind the scenes, but if we need to index it again, can we add it back to the mapping?
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 will remove the explicit mappings for all fields except for events
and enabled
since we filter on those.
MAINTENANCE_WINDOW_SAVED_OBJECT_TYPE, | ||
} from '../common'; | ||
|
||
export const maintenanceWindowFeature: KibanaFeatureConfig = { |
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.
Nicely done!
import { createValidateRruleBy } from '../../lib/validate_rrule_by'; | ||
import { validateSnoozeStartDate, validateSnoozeEndDate } from '../../lib/validate_snooze_date'; | ||
|
||
export const rRuleSchema = schema.object({ |
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.
That's great to have!
context: MaintenanceWindowClientContext, | ||
params: ArchiveParams | ||
): Promise<MaintenanceWindow> { | ||
return await retryIfConflicts( |
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 am wondering if we really need to do that or just use the saved object options retryOnConflict
. They already doing it for us what do you think?
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 don't think we can specify retryOnConflict
for update calls on the savedObjectClient when version
is defined.
In the last commit d65b6a9, I made the following changes:
|
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.
Code looks good and working as expected. Please create an issue tech debt to bring back all the maintenance window files under one folder alerting/server/maintenance_window
Awesome work here!!!
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Public APIs missing exports
Page load bundle
Saved Objects .kibana field count
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
…Fields (#154761) ## Summary Resolves: #153468 Maintenance window API PR: #153411 This PR does the following: - Skip alert notifications for rules in maintenance - Add `maintenance_window_ids` field to alert events in the event log - Add `maintenance_window_ids` attribute to AAD ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: kibanamachine <[email protected]>
Summary
Resolves: #152270
Specs: https://docs.google.com/document/u/1/d/1-QblF6P19W9o5-10Us3bfgN80GRfjSIhybHrvJS_ObA/edit
This PR implements the following:
This PR does not include integration with task runner, a new PR will be created to do that work.
APIs:
Maintenance window response schema: