-
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] Telemetry: track analysis endpoint usage #166988
[ML][AIOps] Telemetry: track analysis endpoint usage #166988
Conversation
Pinging @elastic/ml-ui (:ml) |
55a7ce3
to
a71ecb5
Compare
cc @benakansara for the infra plugin addition. |
.../public/components/log_rate_analysis/log_rate_analysis_content/log_rate_analysis_content.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/aiops/public/components/log_rate_analysis/log_rate_analysis_results.tsx
Outdated
Show resolved
Hide resolved
@@ -268,6 +268,7 @@ export const LogRateAnalysis: FC<AlertDetailsLogRateAnalysisSectionProps> = ({ r | |||
</EuiFlexItem> | |||
<EuiFlexItem> | |||
<LogRateAnalysisContent | |||
source="observability_alerts" |
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.
Maybe observability_alert_details
or log_threshold_alert_details
?
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.
Updated in 8c6bc9d
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.
export const LOG_RATE_ANALYSIS_RUN = 'aiops_log_rate_analysis_run'; | ||
|
||
export const LOG_PATTERN_ANALYSIS_RUN = 'aiops_log_pattern_analysis_run'; | ||
|
||
export const CHANGE_POINT_DETECTION_RUN = 'aiops_change_point_detection_run'; | ||
|
||
export const AIOPS_DEFAULT_SOURCE = 'ml_aiops_labs'; |
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.
Suggest to make these single consts an enum like object with as const
, something like:
export const AIOPS_TELEMETRY_ID = {
LOG_RATE_ANALYSIS_RUN: ...,
LOG_PATTERN_ANALYSIS_RUN: ...
} as const;
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.
Updated in bc27b46
@@ -64,6 +65,8 @@ export interface LogRateAnalysisContentProps { | |||
barHighlightColorOverride?: string; | |||
/** Optional callback that exposes data of the completed analysis */ | |||
onAnalysisCompleted?: (d: LogRateAnalysisResultsData) => void; | |||
/** Optional identifier to indicate the plugin utilizing the component */ | |||
source?: string; |
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 wonder if we should make this non-optional to enforce correct telemetry on new/future embeddings?
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 call - updated in bc27b46
@@ -268,6 +268,7 @@ export const LogRateAnalysis: FC<AlertDetailsLogRateAnalysisSectionProps> = ({ r | |||
</EuiFlexItem> | |||
<EuiFlexItem> | |||
<LogRateAnalysisContent | |||
source="observability_alert_details" |
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 have other alert details pages for other rule types. Could we add a prefix/suffix to specify this is related to Log threshold?
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.
Added in bc27b46
@elasticmachine merge upstream |
This has been updated and is ready for another look when you get a chance 🙏 cc @walterra, @peteharverson, @fkanout |
CHANGE_POINT_DETECTION_RUN: 'aiops_change_point_detection_run', | ||
AIOPS_DEFAULT_SOURCE: 'ml_aiops_labs', | ||
AIOPS_ANALYSIS_RUN_ORIGIN: 'aiops-analysis-run-origin', | ||
} as const; |
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.
Sorry for the continued nitpicking 😅 — looking at the attributes now they serve different purposes so we probably shouldn't combine them all into one enum. Suggest to break out the run
ones.
If I read the code right AIOPS_ANALYSIS_RUN_ORIGIN
is now the header name? Suggest to prefix with kbn-
then. We now also have a mix of source
and origin
in the naming. Related to that, if you align this, maybe we can also make the prop name source
more specific, 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.
The header name can't begin with 'kbn' - I got an error. But good call on renaming the prop to match up.
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.
Renamed prop name to 'embeddingOrigin' in c973e76
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.
@@ -32,6 +36,13 @@ export const defineLogCategorizationRoutes = ( | |||
}, | |||
}, | |||
async (context, request, response) => { | |||
const { headers } = request; | |||
trackAIOpsRouteUsage( |
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.
@jgowdyelastic is this in the right place? We want to track the number of times the analysis is run from the ML AIOps Labs Log Pattern Analysis page, or from within Discover. Currently I don't see this counter incrementing when I run the analysis from either page.
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 endpoint will be called whenever the analysis is run, but it also has the risk that it could be called without the categorization analysis being run.
Debugging this, it appears there is a bug. The AIOPS_TELEMETRY_ID.AIOPS_ANALYSIS_RUN_ORIGIN
header is not being added to the request and so the telemetry is not being counted.
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.
One alternative would be to also implement client side telemetry to count this on click of the analyse button or in the case of the Discover embedded version, on render of the component.
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.
Fixed categorization tracking in c973e76
Did some more testing and was able to generate the following data with the latest state:
Looks good IMHO! When I run log pattern analysis via Discover it still tracks it as |
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 latest state correctly tracks usage of log pattern analysis across AIOps Labs/Discover:
{
"domainId": "aiops",
"counterName": "POST /internal/aiops/categorization_field_validation",
"counterType": "run_via_discover_run_pattern_analysis",
"lastUpdatedAt": "2023-09-27T17:16:35.721Z",
"fromTimestamp": "2023-09-27T00:00:00Z",
"total": 1
},
{
"domainId": "aiops",
"counterName": "POST /internal/aiops/categorization_field_validation",
"counterType": "run_via_ml_aiops_labs",
"lastUpdatedAt": "2023-09-27T17:16:15.710Z",
"fromTimestamp": "2023-09-27T00:00:00Z",
"total": 4
},
Just a note: I think it's good enough for now, but should the log pattern analysis flyout ever be used in multiple places we need to make sure that the origin gets passed on from the place where it is embedded from. The way the code is structured now, it would report any flyout usage as a trigger via Discover.
@@ -70,6 +70,7 @@ export async function showCategorizeFlyout( | |||
savedSearch={null} | |||
selectedField={field} | |||
onClose={onFlyoutClose} | |||
embeddingOrigin={'discover_run_pattern_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.
Unfortunately I don't think hardcoding the ID here is sufficient. This flyout could be (in the future) opened from other places in kibana.
To get the real origin of the action, you'll need to have the value passed into this component as a prop and set by the ui action that has trigged it.
The context that is passed to the action's execute function does have an originatingApp
variable which I added just in case when this was first written.
This has the value discover
when called from Discover.
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. Tested analysis endpoint with Log threshold alert details page.
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.
Tested and 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.
LGTM
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Summary
This PR adds tracking for Log Rate Analysis and Log Pattern Analysis endpoints for AIOps.