Skip to content
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

Added workflow as a composite monitor #380

Conversation

stevanbz
Copy link
Contributor

Description

Added workflow

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Stevan Buzejic <buzejic.stevan@gmail.com>
@stevanbz stevanbz requested a review from a team March 13, 2023 22:11
val workflowId: String
val version: Long
val method: RestRequest.Method
val srcContext: FetchSourceContext?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why FetchSourceContext type and not a specific data model?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh - it's copy/paste from GetMonitorRequest

data class Delegate(
val order: Int,
val monitorId: String,
val chainedFindings: ChainedFindings? = null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This points to one monitor, do we want a list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think no. Each monitor that is currently being processed by the runner will have chainedFindings containing only one monitor - the one that was executed just before. By using the monitor id and the current workflow execution id, we will be able to find the findings (and extract the docIds) - which we can use as a filter for the current monitor execution.

Maybe the property is not named correctly - in general in chainedFindings we are not storing the monitor composition but in inputs property.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh ok, I think we should rename it to chainedFinding. Making it plural implies there should be more.

val user: User?,
val schemaVersion: Int = NO_SCHEMA_VERSION,
val inputs: List<WorkflowInput>,
val owner: String? = "alerting"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this a constant like DEFAULT_OWNER?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Tnx!

/**
* Context passed in delegate monitor to filter data queried by a monitor based on the findings of the given monitor id.
*/
data class ChainedFindings(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to ChainedFinding

…ding to conversation on the PR

Signed-off-by: Stevan Buzejic <buzejic.stevan@gmail.com>
* @param request The request object
* @param listener The listener for getting response
*/
fun getWorkflow(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can also add execute workflow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could - but then we need to extract the ExecuteWorkflowRequest.
Should I add a execute workflow method here or?

class DeleteWorkflowRequest : ActionRequest {

val workflowId: String
val deleteDelegateMonitors: Boolean?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java docs


val workflowId: String
val deleteDelegateMonitors: Boolean?
val refreshPolicy: WriteRequest.RefreshPolicy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove refresh policy
we shoudl alway set it as IMMEDIATE and not take input from user


class GetWorkflowRequest : ActionRequest {
val workflowId: String
val version: Long
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove version from request. User should be able to fetch by id

/**
* Context passed in delegate monitor to filter data queried by a monitor based on the findings of the given monitor id.
*/
// TODO - Remove the class and move the monitorId to Delegate (as a chainedMonitorId property) if this class won't be updated by adding new properties
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the TODO resolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - I just added TODO according to what I discussed with Ashish:
"Yea we can leave it as is, but add a TODO to mention if this class will really be needed in the future if it wont get updated."

* Optionally accepts chained findings context.
* */
data class Delegate(
val order: Int,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java docs for field params

@@ -21,9 +21,29 @@ class Finding(
val monitorName: String,
val index: String,
val docLevelQueries: List<DocLevelQuery>,
val timestamp: Instant
val timestamp: Instant,
val executionId: String? = null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java docs

override val schedule: Schedule,
override val lastUpdateTime: Instant,
override val enabledTime: Instant?,
// TODO: Check how this behaves during rolling upgrade/multi-version cluster
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the todo resolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's ok we can test this before merging feature branch into main

…ed properties

Signed-off-by: Stevan Buzejic <buzejic.stevan@gmail.com>
Signed-off-by: Stevan Buzejic <buzejic.stevan@gmail.com>
@eirsep eirsep merged commit 42a2d4d into opensearch-project:feature/composite-monitors Mar 21, 2023
stevanbz added a commit to stevanbz/common-utils that referenced this pull request Apr 17, 2023

Signed-off-by: Stevan Buzejic <buzejic.stevan@gmail.com>
eirsep pushed a commit to eirsep/common-utils that referenced this pull request May 24, 2023
Signed-off-by: Stevan Buzejic <buzejic.stevan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants