-
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
[ML] AIOps: Renames Explain Log Rate Spikes to Log Rate Analysis. #161764
[ML] AIOps: Renames Explain Log Rate Spikes to Log Rate Analysis. #161764
Conversation
Documentation preview: |
3c5678d
to
886ae22
Compare
bca93e1
to
e60562b
Compare
e60562b
to
69e975b
Compare
Pinging @elastic/ml-ui (:ml) |
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.
Thank you for handling all the docs-related changes, too! Docs LGTM!
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.
Apart from the minor comments, LGTM. Gave it a quick test and couldn't see any leftover references to 'Explain Log Rate Spikes'.
@@ -38,4 +38,4 @@ export const aiopsExplainLogRateSpikesSchema = schema.object({ | |||
sampleProbability: schema.maybe(schema.number()), | |||
}); | |||
|
|||
export type AiopsExplainLogRateSpikesSchema = TypeOf<typeof aiopsExplainLogRateSpikesSchema>; | |||
export type AiopsLogRateAnalysisSchema = TypeOf<typeof aiopsLogRateAnalysisSchema>; |
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.
Nit - there is a comment in this file which refers to spikes
. I assume that info on dips
will be stored in it too?
@@ -170,7 +170,7 @@ export const ExplainLogRateSpikesContent: FC<ExplainLogRateSpikesContentProps> = | |||
title={ | |||
<h2> | |||
<FormattedMessage | |||
id="xpack.aiops.explainLogRateSpikesPage.emptyPromptTitle" | |||
id="xpack.aiops.logRateAnalysis.page.emptyPromptTitle" | |||
defaultMessage="Click a spike in the histogram chart to start the analysis." |
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.
Did you decide to leave out drop
from this one until the functionality for drops is implemented?
@@ -384,7 +383,7 @@ export const ExplainLogRateSpikesAnalysis: FC<ExplainLogRateSpikesAnalysisProps> | |||
body={ | |||
<p> | |||
<FormattedMessage | |||
id="xpack.aiops.explainLogRateSpikesPage.noResultsPromptBody" | |||
id="xpack.aiops.logRateAnalysis.page.noResultsPromptBody" | |||
defaultMessage="Try to adjust the baseline and deviation time ranges and rerun the analysis. If you still get no results, there might be no statistically significant entities contributing to this spike in log rates." |
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.
Are you leaving out drop
from this one until the drop functionality is implemented?
@peteharverson Thanks for spotting those UI texts, I added a note to the meta issue (#161832) to update them once we add support for drops in the ML plugin page. |
"Explain Log Rate Spikes" is an AIOps feature that uses advanced statistical methods to identify reasons for increases in log rates. It makes it easy to find and investigate causes of unusual spikes by using the analysis workflow view. | ||
You are using "Explain Log Rate Spikes" and ran the statistical analysis on the log messages which occured during the alert. | ||
You received the following analysis results from "Explain Log Rate Spikes" which list statistically significant co-occuring field/value combinations sorted from most significant (lower p-values) to least significant (higher p-values) that contribute to the log messages spike: | ||
"Log Rate Analysis" is an AIOps feature that uses advanced statistical methods to identify reasons for increases in log rates. It makes it easy to find and investigate causes of unusual spikes by using the analysis workflow view. |
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.
@walterra Making a note, we might need to change prompt text once drop functionality is implemented.
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.
Good point, I also added a note to our meta issue regarding drop support: #161832
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.
AO changes LGTM
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.
Changes in x-pack/plugins/serverless_observability/public/components/side_navigation/index.tsx
LGTM
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.
Serverless oblt changes LGTM
@@ -63,23 +63,24 @@ const PROGRESS_STEP_GROUPING = 0.1; | |||
const PROGRESS_STEP_HISTOGRAMS = 0.1; | |||
const PROGRESS_STEP_HISTOGRAMS_GROUPS = 0.1; | |||
|
|||
export const defineExplainLogRateSpikesRoute = ( | |||
export const defineLogRateAnalysisRoute = ( |
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 having the path passed in to a common route registering function is correct way to do this.
In the future the LOG_RATE_ANALYSIS
route will probably change and have its version bumped, and the old route should not change. However as it is now, the old route is the same as the new route and so it will also change versions.
I think the correct way to do this would be to have two completely separate route registrations, and after some time we can delete the deprecated one.
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.
Thinking about this more, I'm now doubting myself.
It probably will be ok to leave as is, as users won't have this server side endpoint bookmarked. But that also makes me wonder if we need to keep the old one around at all during this rename?
In the future, just deleting deprecated code is easier than having to delete and change this function to remove the path
variable. So maybe having a completely separate route registrations is cleaner? Even if it is identical code.
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.
As discussed offline, I removed the deprecated API endpoint in a532e49. It was good as a dry run how this might play out in future serverless environments, but it's not necessary to keep the old endpoint around yet since it's internal and experimental.
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
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @walterra |
…stic#161764) ## Summary Part of elastic#161832. This PR renames the Explain Log Rate Spikes feature to **Log Rate Analysis**. - [x] Renamed references in `docs/developer/*` - [x] Updated docs screenshots - [x] Redirect in docs - [x] Redirect urls from `explain_log_rate_spikes` to `log_rate_analysis` - [x] API versioning - [x] Renamed navigation links - [x] Renamed variable names - [x] Renamed file names - [x] Renamed i18n ids - [x] Renamed breadcrumbs - [x] Removed hard coded `AIOPS_ENABLED` feature flag
…stic#161764) ## Summary Part of elastic#161832. This PR renames the Explain Log Rate Spikes feature to **Log Rate Analysis**. - [x] Renamed references in `docs/developer/*` - [x] Updated docs screenshots - [x] Redirect in docs - [x] Redirect urls from `explain_log_rate_spikes` to `log_rate_analysis` - [x] API versioning - [x] Renamed navigation links - [x] Renamed variable names - [x] Renamed file names - [x] Renamed i18n ids - [x] Renamed breadcrumbs - [x] Removed hard coded `AIOPS_ENABLED` feature flag
Summary
Part of #161832.
This PR renames the Explain Log Rate Spikes feature to Log Rate Analysis.
docs/developer/*
explain_log_rate_spikes
tolog_rate_analysis
AIOPS_ENABLED
feature flagChecklist