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

[APM] Use model_plot as a signal for anomaly scores #77756

Merged
merged 3 commits into from
Sep 21, 2020

Conversation

dgieselaar
Copy link
Member

Closes #77083.

Before:
image
image

After:
image
image

@dgieselaar dgieselaar requested a review from a team as a code owner September 17, 2020 12:36
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

small nits about inlining. Other than that 👍

Comment on lines 56 to 58
const lte = end;
// fetch data for at least 30 minutes
const gte = Math.min(end - 30 * 60 * 1000, start);
Copy link
Member

Choose a reason for hiding this comment

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

I think these would read better if they were inlined. A const named "lte" without context is pretty cryptic.

Comment on lines 154 to 157
transactionType,
jobId,
actualValue: mlResult?.actual?.[0],
anomalyScore: mlResult?.record_score || 0,
Copy link
Member

Choose a reason for hiding this comment

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

nit: any reason some of these are inlined and others are not?

Suggested change
transactionType,
jobId,
actualValue: mlResult?.actual?.[0],
anomalyScore: mlResult?.record_score || 0,
transactionType: mlResult?.job_id,
jobId: mlResult?.by_field_value,
actualValue: mlResult?.actual?.[0],
anomalyScore: mlResult?.record_score || 0,

@@ -130,13 +143,15 @@ function transformResponseToServiceAnomalies(
response.aggregations?.services.buckets ?? []
).reduce(
(statsByServiceName, { key: serviceName, top_score: topScoreAgg }) => {
const mlResult = topScoreAgg.hits.hits[0]._source;
Copy link
Member

@sorenlouv sorenlouv Sep 18, 2020

Choose a reason for hiding this comment

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

You had this as a maybe previously? Was that not the case?
(I like it better now for sure)

Copy link
Member Author

@dgieselaar dgieselaar Sep 18, 2020

Choose a reason for hiding this comment

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

It's always there, because we don't use min_doc_count: 0. So the maybe here was too defensive anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Great 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@dgieselaar dgieselaar merged commit ce7922b into elastic:master Sep 21, 2020
@dgieselaar dgieselaar deleted the service-health-anomalies branch September 21, 2020 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] Services without anomalies have an "unknown" health status
4 participants