[APM] Use status_code field to calculate error rate by cauemarcondes · Pull Request #71109 · elastic/kibana

cauemarcondes · 2020-07-08T15:25:36Z

elasticmachine · 2020-07-08T15:25:38Z

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

x-pack/plugins/apm/server/lib/errors/get_error_rate.ts

sorenlouv · 2020-07-08T16:05:21Z

x-pack/plugins/apm/server/lib/errors/get_error_rate.ts

+// Regex for 5xx and 4xx
+const errorStatusCodeRegex = /5\d{2}|4\d{2}/;
+
 export async function getErrorRate({


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

dgieselaar · 2020-07-08T18:18:31Z

`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 · 2020-07-08T18:22:10Z

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 · 2020-07-08T18:24:06Z

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

cauemarcondes · 2020-07-08T18:25:47Z

@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 · 2020-07-08T18:27:49Z

@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.

x-pack/plugins/apm/server/lib/errors/get_error_rate.ts

x-pack/plugins/apm/server/routes/errors.ts

sorenlouv

lgtm 👍

cauemarcondes · 2020-07-13T10:58:24Z

lgtm 👍

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

sorenlouv · 2020-07-13T11:54:45Z

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

Can you double check with @nehaduggal first?

nehaduggal · 2020-07-13T13:46:11Z

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

dgieselaar · 2020-07-13T13:49:08Z

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

cauemarcondes · 2020-07-13T15:53:01Z

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 · 2020-07-13T16:04:08Z

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 · 2020-07-14T07:18:04Z

@elasticmachine merge upstream

kibanamachine · 2020-07-14T09:13:06Z

💚 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

* calculating error rate based on status code * fixing unit test * addressing pr comments * adding erroneous transactions rate * adding erroneous transactions rate * adding error rate to detail page * fixing i18n Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

sorenlouv · 2020-07-14T10:20:37Z

💚 Backport successful

The PR was backported to the following branches:

✅ 7.x

* master: (314 commits) [APM] Use status_code field to calculate error rate (elastic#71109) [Observability] Change appLink passing the date range (elastic#71259) [Security] Add Timeline improvements (elastic#71506) adjust vislib bar opacity (elastic#71421) Fix ScopedHistory mock and adapt usages (elastic#71404) [Security Solution] Add hook for reading/writing resolver query params (elastic#70809) [APM] Bug fixes from ML integration testing (elastic#71564) [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404) [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275) [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321) Change signal.rule.risk score mapping from keyword to float (elastic#71126) Added help text where needed on connectors and alert actions UI (elastic#69601) [SIEM][Detections] Value Lists Management Modal (elastic#67068) [test] Skips test preventing promotion of ES snapshot elastic#71582 [test] Skips test preventing promotion of ES snapshot elastic#71555 [ILM] Fix alignment of the timing field (elastic#71273) [SIEM][Detection Engine][Lists] Adds the ability for exception lists to be multi-list queried. (elastic#71540) initial telemetry setup (elastic#69330) [Reporting] Formatting fixes for CSV export in Discover, CSV download from Dashboard panel (elastic#67027) Search across spaces (elastic#67644) ...

…t-apps-page-titles * 'master' of github.com:elastic/kibana: (88 commits) [ML] Functional tests - disable DFA creation and cloning tests [APM] Use status_code field to calculate error rate (elastic#71109) [Observability] Change appLink passing the date range (elastic#71259) [Security] Add Timeline improvements (elastic#71506) adjust vislib bar opacity (elastic#71421) Fix ScopedHistory mock and adapt usages (elastic#71404) [Security Solution] Add hook for reading/writing resolver query params (elastic#70809) [APM] Bug fixes from ML integration testing (elastic#71564) [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404) [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275) [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321) Change signal.rule.risk score mapping from keyword to float (elastic#71126) Added help text where needed on connectors and alert actions UI (elastic#69601) [SIEM][Detections] Value Lists Management Modal (elastic#67068) [test] Skips test preventing promotion of ES snapshot elastic#71582 [test] Skips test preventing promotion of ES snapshot elastic#71555 [ILM] Fix alignment of the timing field (elastic#71273) [SIEM][Detection Engine][Lists] Adds the ability for exception lists to be multi-list queried. (elastic#71540) initial telemetry setup (elastic#69330) [Reporting] Formatting fixes for CSV export in Discover, CSV download from Dashboard panel (elastic#67027) ... # Conflicts: # x-pack/plugins/index_management/public/application/index.tsx

* master: (72 commits) [test] Skips test preventing promotion of ES snapshot elastic#71612 [Logs UI] Remove UUID from Alert Instances (elastic#71340) [Metrics UI] Remove UUID from Alert Instance IDs (elastic#71335) [ML] Functional tests - disable DFA creation and cloning tests [APM] Use status_code field to calculate error rate (elastic#71109) [Observability] Change appLink passing the date range (elastic#71259) [Security] Add Timeline improvements (elastic#71506) adjust vislib bar opacity (elastic#71421) Fix ScopedHistory mock and adapt usages (elastic#71404) [Security Solution] Add hook for reading/writing resolver query params (elastic#70809) [APM] Bug fixes from ML integration testing (elastic#71564) [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404) [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275) [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321) Change signal.rule.risk score mapping from keyword to float (elastic#71126) Added help text where needed on connectors and alert actions UI (elastic#69601) [SIEM][Detections] Value Lists Management Modal (elastic#67068) [test] Skips test preventing promotion of ES snapshot elastic#71582 [test] Skips test preventing promotion of ES snapshot elastic#71555 [ILM] Fix alignment of the timing field (elastic#71273) ...

* calculating error rate based on status code * fixing unit test * addressing pr comments * adding erroneous transactions rate * adding erroneous transactions rate * adding error rate to detail page * fixing i18n Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Cauê Marcondes <55978943+cauemarcondes@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

dgieselaar · 2020-08-05T09:14:01Z

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.

cauemarcondes added 2 commits July 8, 2020 17:17

calculating error rate based on status code

bd1c0ad

fixing unit test

614ac9b

cauemarcondes added Team:APM - DEPRECATED Use Team:obs-ux-infra_services. release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jul 8, 2020

cauemarcondes requested a review from a team July 8, 2020 15:25

sorenlouv reviewed Jul 8, 2020

View reviewed changes

x-pack/plugins/apm/server/lib/errors/get_error_rate.ts Outdated Show resolved Hide resolved

sorenlouv reviewed Jul 8, 2020

View reviewed changes

x-pack/plugins/apm/server/lib/errors/get_error_rate.ts Outdated Show resolved Hide resolved

sorenlouv reviewed Jul 8, 2020

View reviewed changes

addressing pr comments

8829034

sorenlouv reviewed Jul 9, 2020

View reviewed changes

x-pack/plugins/apm/server/lib/errors/get_error_rate.ts Outdated Show resolved Hide resolved

sorenlouv reviewed Jul 9, 2020

View reviewed changes

x-pack/plugins/apm/server/lib/errors/get_error_rate.ts Show resolved Hide resolved

cauemarcondes added 3 commits July 13, 2020 10:34

adding erroneous transactions rate

6699639

Merge branch 'master' of github.com:elastic/kibana into error-rate

09f3146

adding erroneous transactions rate

50676a7

sorenlouv reviewed Jul 13, 2020

View reviewed changes

x-pack/plugins/apm/server/routes/errors.ts Outdated Show resolved Hide resolved

sorenlouv approved these changes Jul 13, 2020

View reviewed changes

adding error rate to detail page

a2b6c94

Merge branch 'master' of github.com:elastic/kibana into error-rate

28e053c

fixing i18n

32a35a0

Merge branch 'master' into error-rate

10c760e

cauemarcondes merged commit 90f233b into elastic:master Jul 14, 2020

cauemarcondes deleted the error-rate branch July 14, 2020 10:20

sorenlouv mentioned this pull request Jul 14, 2020

[7.x] [APM] Use status_code field to calculate error rate (#71109) #71608

Merged

cauemarcondes added the apm-test-plan-7.9.0 label Jul 20, 2020

dgieselaar self-assigned this Aug 4, 2020

dgieselaar added the apm:test-plan-done Pull request that was successfully tested during the test plan label Aug 5, 2020

Conversation

cauemarcondes commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jul 8, 2020

Uh oh!

Uh oh!

Uh oh!

sorenlouv Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

dgieselaar commented Jul 8, 2020 via email

Uh oh!

cauemarcondes commented Jul 8, 2020

Uh oh!

dgieselaar commented Jul 8, 2020

Uh oh!

cauemarcondes commented Jul 8, 2020

Uh oh!

dgieselaar commented Jul 8, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sorenlouv left a comment

Choose a reason for hiding this comment

Uh oh!

cauemarcondes commented Jul 13, 2020

Uh oh!

sorenlouv commented Jul 13, 2020

Uh oh!

nehaduggal commented Jul 13, 2020

Uh oh!

dgieselaar commented Jul 13, 2020

Uh oh!

cauemarcondes commented Jul 13, 2020

Uh oh!

dgieselaar commented Jul 13, 2020

Uh oh!

cauemarcondes commented Jul 14, 2020

Uh oh!

kibanamachine commented Jul 14, 2020

💚 Build Succeeded

Build metrics

History

Uh oh!

sorenlouv commented Jul 14, 2020

💚 Backport successful

Uh oh!

dgieselaar commented Aug 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cauemarcondes commented Jul 8, 2020 •

edited

Loading