-
Notifications
You must be signed in to change notification settings - Fork 106
PPL Alerting with Feature Flag #1311
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
base: main
Are you sure you want to change the base?
Conversation
…ject#1295) (cherry picked from commit a232821) Signed-off-by: opensearch-ci <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…opensearch-project#1297) (cherry picked from commit 147e2e7) Signed-off-by: Peter Zhu <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Revert "fix mustache format notif message" This reverts commit 862d635. update tests Signed-off-by: KashKondaka <[email protected]>
Signed-off-by: KashKondaka <[email protected]>
public/services/services.ts
Outdated
| if (alertingDashboardsCap?.pplV2 === true || alertingCap?.pplV2 === true) { | ||
| return true; | ||
| } | ||
| return true; |
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.
keep this false
public/services/services.ts
Outdated
| return true; | ||
| } | ||
| const alertingDashboardsCap = capabilities.alertingDashboards; | ||
| const alertingCap = capabilities.alerting; |
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.
remove this one and also simply return !!capabilities.alertingDashboards?.pplV2
public/services/services.ts
Outdated
| if (!isPplV2Enabled()) { | ||
| return false; | ||
| } |
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.
Remove this since this check is not ppl alerting and should exist regardless
Signed-off-by: KashKondaka <[email protected]>
Signed-off-by: KashKondaka <[email protected]>
Signed-off-by: KashKondaka <[email protected]>
Signed-off-by: KashKondaka <[email protected]>
server/plugin.js
Outdated
| destinations(services, router, dataSourceEnabled); | ||
| opensearch(services, router, dataSourceEnabled); | ||
| monitors(services, router, dataSourceEnabled); | ||
| pplAlertingMonitors(services, router, dataSourceEnabled); |
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.
gate this and remove the per api gating
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.
ack
server/routes/monitors.js
Outdated
| monitorService.getMonitors | ||
| ); | ||
|
|
||
| router.get( |
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.
Why do we have this?
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.
shouldn't have it removed
| export type { ExplorePluginSetup, ExplorePluginStart } from '../../../src/plugins/explore/public'; | ||
| // @ts-ignore | ||
| export type { QueryWithQueryAsString } from '../../../src/plugins/explore/public'; |
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.
We shouldn't be exporting these from this plugin?
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 are needed for alert flyout in discover.
Signed-off-by: KashKondaka <[email protected]>
public/plugin.tsx
Outdated
| // mount: async (params) => { | ||
| // const { renderApp } = await import('./app'); | ||
| // const [coreStart] = await core.getStartServices(); | ||
| // return renderApp(coreStart, params); | ||
| // }, |
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.
Remove?
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.
other places too
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.
ack removed
| order: 9070, | ||
| category: DEFAULT_APP_CATEGORIES.detect, | ||
| updater$: this.appStateUpdater, | ||
| updater$: this.appStateUpdater$ as any, |
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.
why did we make this change?
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.
was getting a type error because the navigation API expects an Observable, not the BehaviorSubject instance itself. BehaviorSubject implements the observable interface, but its public type also exposes .next() and .value which doesn't match watch the registry expects
Signed-off-by: KashKondaka <[email protected]>
|
|
||
| public start(core: CoreStart, { visAugmenter, embeddable, data, navigation, contentManagement, assistantDashboards }: AlertingStartDeps): AlertingStart { | ||
| public start(core: CoreStart, { visAugmenter, embeddable, data, navigation, contentManagement, assistantDashboards, explore }: AlertingStartDeps): AlertingStart { | ||
| navigateToAppRef = core.application.navigateToApp; |
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.
where are we using this? and why do we need to create a variable for this?
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 was old code for transporting query to ppl alerting. removing it
public/utils/ppl_alerting_support.ts
Outdated
| return { status: 'unknown' }; | ||
| }; | ||
|
|
||
| export const ensurePplSupport = ( |
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.
what is this used for?
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 actually being used, will remove
public/services/services.ts
Outdated
| if (!isPplAlertingEnabled()) { | ||
| return false; | ||
| } |
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 remove this since updated ux should not be gated on ppl alerting
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.
When I remove this it looks like updated ux overwrites ppl flag
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.
found error, fixed
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.
Can you change all new files to be ts(x)?
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.
will address in next pr
Signed-off-by: KashKondaka <[email protected]>
Signed-off-by: KashKondaka <[email protected]>
Signed-off-by: KashKondaka <[email protected]>
Signed-off-by: KashKondaka <[email protected]>
Signed-off-by: KashKondaka <[email protected]>
Signed-off-by: KashKondaka <[email protected]>
Signed-off-by: KashKondaka <[email protected]>
Signed-off-by: KashKondaka <[email protected]>
Signed-off-by: KashKondaka <[email protected]>
Signed-off-by: KashKondaka <[email protected]>
Description
PPL Alerting changes
Issues Resolved
[List any issues this PR will resolve]
Check List
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.