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] Move impact calculation to Elasticsearch #26436

Merged

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Nov 29, 2018

Closes #26428

This moves the calculation of impact from the Nodejs to Elasticsearch.
Impact used to be calculated as transactionPerMinute * avgResponseTime. The equivalent in ES is to sum avg response times.

Calculating and sorting by impact in ES ensures that the top 100 buckets returned are actually the top 100 most impactful endpoints - and not just the top 100 slowest endpoints.

This will be backported to 6.5.

cc @roncohen

@sorenlouv sorenlouv added the Team:APM All issues that need APM UI Team support label Nov 29, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv
Copy link
Member Author

And here is the manual backport #26443

};
avg: { value: number };
p95: { values: { '95.0': number } };
sum: { value: number };
Copy link
Member Author

Choose a reason for hiding this comment

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

I find the one-liners much more readable.

@@ -21,6 +21,7 @@ export const transactionGroupsResponse = ({
doc_count: 180,
avg: { value: 255966.30555555556 },
p95: { values: { '95.0': 320238.5 } },
sum: { value: 46073935 },
Copy link
Member Author

@sorenlouv sorenlouv Nov 29, 2018

Choose a reason for hiding this comment

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

No, these sum values are not just random numbers. They are doc_count * avg.value for each bucket. Was hoping the test snapshot would stay consistent but if you look at the changed impact values the differences are very small.

@sorenlouv sorenlouv force-pushed the move-impact-calc-to-elasticsearch branch from 560f55a to 0893dc6 Compare November 29, 2018 22:46
@elasticmachine
Copy link
Contributor

💔 Build Failed

@roncohen
Copy link
Contributor

would be cool to try it out on some real data to make sure it works before merging?

@sorenlouv
Copy link
Member Author

@roncohen I already verified that it produces the same impact score as before (for opbeans data). Would be great to try it against elastic.co data but not sure how I can access that locally.

@sorenlouv sorenlouv force-pushed the move-impact-calc-to-elasticsearch branch from 0893dc6 to c294681 Compare November 30, 2018 09:36
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv sorenlouv force-pushed the move-impact-calc-to-elasticsearch branch from c294681 to 9ed6ad0 Compare December 3, 2018 14:57
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

aggregations: {
transactions: { buckets: [getBucket(10), getBucket(20), getBucket(50)] }
}
} as unknown) as ESResponse;
Copy link
Contributor

Choose a reason for hiding this comment

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

why is as unknown necessary when it get interpreted as ESResponse later on?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without first casting it to unknown, it complains because the object contains properties that are not defined on ESResponse. I think this is due to @types/elasticsearch not having everything defined properly.

I wish we didn't have to do this, because it's definitely defeating the purpose of a type system, but I think it's the most pragmatic solution for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the tsc error I get without unknown:

Conversion of type '{ took: number; timed_out: false; _shards: { total: number; successful: number; skipped: number; failed: number; }; hits: { total: number; max_score: null; hits: never[]; }; aggregations: { transactions: { ...; }; }; }' to type 'AggregationSearchResponse<void, Aggs>' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
  Types of property 'aggregations' are incompatible.
    Type '{ transactions: { doc_count_error_upper_bound: number; sum_other_doc_count: number; buckets: ({ key: string; doc_count: number; avg: { value: number; }; p95: { values: { '95.0': number; }; }; sample: { hits: { ...; }; }; } | ... 12 more ... | { ...; })[]; }; }' is not comparable to type 'Aggs'.
      Types of property 'transactions' are incompatible.
        Type '{ doc_count_error_upper_bound: number; sum_other_doc_count: number; buckets: ({ key: string; doc_count: number; avg: { value: number; }; p95: { values: { '95.0': number; }; }; sample: { hits: { total: number; max_score: null; hits: { ...; }[]; }; }; } | ... 12 more ... | { ...; })[]; }' is not comparable to type '{ buckets: Bucket[]; }'.
          Types of property 'buckets' are incompatible.
            Type '({ key: string; doc_count: number; avg: { value: number; }; p95: { values: { '95.0': number; }; }; sample: { hits: { total: number; max_score: null; hits: { _index: string; _type: string; _id: string; _score: null; _source: { ...; }; sort: number[]; }[]; }; }; } | ... 12 more ... | { ...; })[]' is not comparable to type 'Bucket[]'.
              Type '{ key: string; doc_count: number; avg: { value: number; }; p95: { values: { '95.0': number; }; }; sample: { hits: { total: number; max_score: null; hits: { _index: string; _type: string; _id: string; _score: null; _source: { ...; }; sort: number[]; }[]; }; }; } | ... 12 more ... | { ...; }' is not comparable to type 'Bucket'.
                Type '{ key: string; doc_count: number; avg: { value: number; }; p95: { values: { '95.0': number; }; }; sample: { hits: { total: number; max_score: null; hits: { _index: string; _type: string; _id: string; _score: null; _source: { ...; }; sort: number[]; }[]; }; }; }' is not comparable to type 'Bucket'.
                  Types of property 'sample' are incompatible.
                    Type '{ hits: { total: number; max_score: null; hits: { _index: string; _type: string; _id: string; _score: null; _source: { '@timestamp': string; processor: { name: "transaction"; event: "transaction"; }; transaction: { ...; }; ... 4 more ...; host: { ...; }; }; sort: number[]; }[]; }; }' is not comparable to type '{ hits: { total: number; max_score: number | null; hits: { _source: Transaction; }[]; }; }'.
                      Types of property 'hits' are incompatible.
                        Type '{ total: number; max_score: null; hits: { _index: string; _type: string; _id: string; _score: null; _source: { '@timestamp': string; processor: { name: "transaction"; event: "transaction"; }; transaction: { ...; }; ... 4 more ...; host: { ...; }; }; sort: number[]; }[]; }' is not comparable to type '{ total: number; max_score: number | null; hits: { _source: Transaction; }[]; }'.
                          Types of property 'hits' are incompatible.
                            Type '{ _index: string; _type: string; _id: string; _score: null; _source: { '@timestamp': string; processor: { name: "transaction"; event: "transaction"; }; transaction: { name: string; duration: { us: number; }; ... 4 more ...; id: string; }; ... 4 more ...; host: { ...; }; }; sort: number[]; }[]' is not comparable to type '{ _source: Transaction; }[]'.
                              Type '{ _index: string; _type: string; _id: string; _score: null; _source: { '@timestamp': string; processor: { name: "transaction"; event: "transaction"; }; transaction: { name: string; duration: { us: number; }; ... 4 more ...; id: string; }; ... 4 more ...; host: { ...; }; }; sort: number[]; }' is not comparable to type '{ _source: Transaction; }'.
                                Object literal may only specify known properties, and '_index' does not exist in type '{ _source: Transaction; }'. [2352]
(property) _index: string

sorenlouv added a commit that referenced this pull request Dec 4, 2018
… | [APM] Sort by score to get transaction samples with sampled:true (#26389) | [APM] Move impact calculation to Elasticsearch (#26436) (#26581)

* [APM] Split API into transactions and transaction_groups (#26349)

* [APM] Sort by score to get transaction samples with sampled:true (#26389)

* [APM] Move impact calculation to Elasticsearch (#26436)

* [APM] Move impact calculation to Elasticsearch

* Renamed “durationSum” to “sum” and went back to single, mutable impact
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:APM All issues that need APM UI Team support v6.5.0 v6.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants