[APM] Use status_code field to calculate error rate

[cauemarcondes]

closes #70223

[elasticmachine]

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

[sorenlouv]

Can you add an api test for GET /api/apm/services/{serviceName}/errors/rate to cover this?

[dgieselaar]

`http.response.status_code` is a long, so you can use a range filter (gte 400) (you can also use it on strings btw).

…

On Wed, Jul 8, 2020 at 7:59 PM Cauê Marcondes ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In x-pack/plugins/apm/server/lib/errors/get_error_rate.ts <#71109 (comment)>: > - size: 0, - query: { - bool: { - filter: [ - ...filter, - ...groupIdFilter, - { term: { [PROCESSOR_EVENT]: ProcessorEvent.error } }, - ], + aggs: { + histogram: { + date_histogram: getMetricsDateHistogramParams(start, end), + aggs: { + statusAggregation: { + terms: { + field: HTTP_RESPONSE_STATUS_CODE, + size: 10, @sqren <https://github.com/sqren> do you for instance have an idea on how to fetch only the erroneous transactions? I can do a terms filter, but I'd have to manually pass all possible status in the array. { "terms": { "http.response.status_code": ["500", "400"...] } } — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#71109 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACWDXDLVSGGOW23NVOLWYDR2SXXNANCNFSM4OUV5H4Q> .

[cauemarcondes]

http.response.status_code is a long, so you can use a range filter (gte 400) (you can also use it on strings btw).

So I could do:

 {
          "range": {
            "http.response.status_code": {
              "gte": 400,
              "lte": 599
            }
          }
        }

[dgieselaar]

@cauemarcondes yes (I'm not sure if it's useful to provide the upper bounds though)

[cauemarcondes]

@cauemarcondes yes (I'm not sure if it's useful to provide the upper bounds though)

Why? But I'm good to use only "gte": 400 too.

[dgieselaar]

@cauemarcondes status codes from 600 and above are not valid, and if they do exist in the data, I'd expect that they should be treated as errors.

[sorenlouv]

lgtm 👍

[cauemarcondes]

lgtm 👍

thanks but I still have to add the error chart in the transaction details page. 😅

[sorenlouv]

thanks but I still have to add the error chart in the transaction details page. 😅

Can you double check with @nehaduggal first?

[nehaduggal]

Lets call it Transaction error rate chart and add it to the transactions page.

[dgieselaar]

Have we considered how this would work with pre-aggregated transactions?

[cauemarcondes]

Have we considered how this would work with pre-aggregated transactions?

We will use the status_code until the agents develop the hasError flag in the transaction document. But I believe we might have the same problem with pre-aggregated transactions, won't we?

[dgieselaar]

has_error is a boolean flag so its impact is limited. http.response.status_code has slightly higher cardinality but it should roughly align with transaction.result so maybe not a cause for concern either. Still, we need to flag this with the APM server team, it should be added there.

I do have some concerns about the things that are being added that are not compatible with pre-aggregated transactions today (the RUM UI is another example). APM Server's release cycle is not tied to Kibana, so it's a risk that the UI expects fields in the pre-aggregated transaction documents that are not there because the user is running an older version of APM Server. That being said, we haven't shipped pre-aggregated transactions yet, so maybe I'm thinking too far ahead.

[cauemarcondes]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 10c760e

Build metrics

✅ unchanged

History

• 💚 Build #60948 succeeded 32a35a0
• 💔 Build #60826 failed a2b6c94
• 💔 Build #60763 failed 50676a7
• 💔 Build #60095 failed 8829034
• 💛 Build #59801 was flaky 614ac9b

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

[sorenlouv]

💚 Backport successful

The PR was backported to the following branches:

• ✅ 7.x

[dgieselaar]

Some conclusions from testing:

• Does not work for RUM data, and only works for transaction.type:request. We should consider hiding the charts in the first case and maybe the latter too?
• The value displayed for average treats no data as 0: [APM] Average for transaction error rate includes null values #74337
• The average series cannot be hidden: [APM] Can't deselect/hide average series for transaction error rate chart #74338
• This implementation uses >=400 to flag a transaction as erroneous, but when the agent flag is implemented, only 500 and up will be treated as such. We should consider fixing this for 7.9 and will discuss it in our weekly. See Add outcome to transactions and spans apm#299.