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.

Sign up for GitHub

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

Jump to bottom

[APM] Use the outcome field to calculate the transaction error rate chart #75528

Merged
merged 16 commits into from
Sep 7, 2020
Merged

[APM] Use the outcome field to calculate the transaction error rate chart #75528

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d39b494
replacing error rate to use event.outcome field
cauemarcondes Aug 20, 2020
4897424
Merge branch 'master' of github.com:elastic/kibana into apm-error-rat…
cauemarcondes Aug 20, 2020
1be58f1
addressing PR comment
cauemarcondes Aug 20, 2020
6db831a
Merge branch 'master' into apm-error-rate-outcome
elasticmachine Aug 20, 2020
e565152
Merge branch 'master' into apm-error-rate-outcome
elasticmachine Aug 24, 2020
1399d86
fixing api test
cauemarcondes Aug 25, 2020
e67cac5
Merge branch 'apm-error-rate-outcome' of github.com:cauemarcondes/kib…
cauemarcondes Aug 25, 2020
817f47a
fixing API test
cauemarcondes Aug 26, 2020
de9b914
Merge branch 'master' of github.com:elastic/kibana into apm-error-rat…
cauemarcondes Aug 26, 2020
5f064ee
Merge branch 'master' into apm-error-rate-outcome
elasticmachine Aug 27, 2020
ade9e9e
Merge branch 'master' into apm-error-rate-outcome
elasticmachine Aug 31, 2020
44f69d9
Merge branch 'master' of github.com:elastic/kibana into apm-error-rat…
cauemarcondes Sep 3, 2020
f459501
fixing api tests
cauemarcondes Sep 3, 2020
2d73328
rmeoving snapshot from api test
cauemarcondes Sep 4, 2020
e9b9642
testing error rate
cauemarcondes Sep 4, 2020
73cd059
Merge branch 'master' into apm-error-rate-outcome
elasticmachine Sep 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions x-pack/plugins/apm/common/__snapshots__/elasticsearch_fieldnames.test.ts.snap
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions x-pack/plugins/apm/common/elasticsearch_fieldnames.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ export const TRANSACTION_SAMPLED = 'transaction.sampled';
export const TRANSACTION_BREAKDOWN_COUNT = 'transaction.breakdown.count';
export const TRANSACTION_PAGE_URL = 'transaction.page.url';

export const EVENT_OUTCOME = 'event.outcome';

export const TRACE_ID = 'trace.id';

export const SPAN_DURATION = 'span.duration.us';
Expand Down
11 changes: 11 additions & 0 deletions x-pack/plugins/apm/common/event_outcome.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

export enum EventOutcome {
success = 'success',
failure = 'failure',
unknown = 'unknown',
}
9 changes: 6 additions & 3 deletions x-pack/plugins/apm/server/lib/transaction_groups/get_error_rate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { mean } from 'lodash';
import { EventOutcome } from '../../../common/event_outcome';
import {
HTTP_RESPONSE_STATUS_CODE,
TRANSACTION_NAME,
TRANSACTION_TYPE,
SERVICE_NAME,
EVENT_OUTCOME,
} from '../../../common/elasticsearch_fieldnames';
import { ProcessorEvent } from '../../../common/processor_event';
import { rangeFilter } from '../../../common/utils/range_filter';
Expand Down Expand Up @@ -42,7 +43,9 @@ export async function getErrorRate({
const filter = [
{ term: { [SERVICE_NAME]: serviceName } },
{ range: rangeFilter(start, end) },
{ exists: { field: HTTP_RESPONSE_STATUS_CODE } },
{
terms: { [EVENT_OUTCOME]: [EventOutcome.failure, EventOutcome.success] },
},
...transactionNamefilter,
...transactionTypefilter,
...uiFiltersES,
Expand All @@ -65,7 +68,7 @@ export async function getErrorRate({
},
aggs: {
erroneous_transactions: {
filter: { range: { [HTTP_RESPONSE_STATUS_CODE]: { gte: 400 } } },
filter: { term: { [EVENT_OUTCOME]: EventOutcome.failure } },
},
},
},
Expand Down
Binary file modified x-pack/test/apm_api_integration/basic/fixtures/es_archiver/8.0.0/data.json.gz
View file
Open in desktop
Binary file not shown.
23 changes: 17 additions & 6 deletions x-pack/test/apm_api_integration/basic/tests/transaction_groups/error_rate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,21 @@
*/
import expect from '@kbn/expect';
import { FtrProviderContext } from '../../../common/ftr_provider_context';
import expectedErrorRate from './expectation/error_rate.json';

export default function ApiTest({ getService }: FtrProviderContext) {
const supertest = getService('supertest');
const esArchiver = getService('esArchiver');

// url parameters
const start = encodeURIComponent('2020-06-29T06:45:00.000Z');
const end = encodeURIComponent('2020-06-29T06:49:00.000Z');
const start = encodeURIComponent('2020-08-25T12:51:30.000Z');
const end = encodeURIComponent('2020-08-25T12:54:30.000Z');
sorenlouv marked this conversation as resolved.
Show resolved Hide resolved
const uiFilters = encodeURIComponent(JSON.stringify({}));

describe('Error rate', () => {
describe('when data is not loaded', () => {
it('handles the empty state', async () => {
const response = await supertest.get(
`/api/apm/services/opbeans-node/transaction_groups/error_rate?start=${start}&end=${end}&uiFilters=${uiFilters}`
`/api/apm/services/opbeans-java/transaction_groups/error_rate?start=${start}&end=${end}&uiFilters=${uiFilters}`
);
expect(response.status).to.be(200);
expect(response.body).to.eql({
Expand All @@ -36,11 +35,23 @@ export default function ApiTest({ getService }: FtrProviderContext) {

it('returns the transaction error rate', async () => {
const response = await supertest.get(
`/api/apm/services/opbeans-node/transaction_groups/error_rate?start=${start}&end=${end}&uiFilters=${uiFilters}`
`/api/apm/services/opbeans-java/transaction_groups/error_rate?start=${start}&end=${end}&uiFilters=${uiFilters}`
);

expect(response.status).to.be(200);
expect(response.body).to.eql(expectedErrorRate);
expect(response.body).to.eql({
noHits: false,
erroneousTransactionsRate: [
Copy link
Member

Choose a reason for hiding this comment

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

IMHO this is not really helpful. Can we avoid a snapshot test? e.g. we can count the buckets, or compare the avg only, etc.

Copy link
Member

@sorenlouv sorenlouv Sep 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, we should spent a little more time finding the right things to test instead of blindly testing the full output (I've been very guilty of this myself). Testing the entire output like this makes the test prone to false positives, where it fails when new properties are added, which leads us to blindly updating it when the implementation changes, thus bugs can creep in.

The challenge by NOT doing snapshot testing is that we don't have as good coverage and bugs can creep in...

I tried to think of the important things to verify, while still making it flexible enough to allow a developer to change the implementation (eg. add new properties).

const { erroneousTransactionsRate } = response.body;

// we want to assert that the range is correct
it("has the correct start date", async () => {
  expect(_.first(erroneousTransactionsRate).x).to.be("...");
});

it("has the correct end date", async () => {
  expect(_.last(erroneousTransactionsRate).x).to.be("...");
});

it("has the correct number of buckets", async () => {
  expect(erroneousTransactionsRate.length).to.be(1337);
});

// we want to assert that the timeseries values align with the average calculated in elasticsearch (not sure they are identical so might not be possible but worth a shot)
it("has the correct calculation for average", async () => {
  const average = _.meanBy(erroneousTransactionsRate, (p) => p.y);
  expect(average).to.be(response.body.average);
});

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like what you suggested @sqren, but I feel that in the test scenarios you described above we are not testing what really matters for the API, that is the error rate. The average is not calculated in ES, it also uses lodash mean, so even if we change the way we calculate the error rate these tests will never fail.

I agree that we should avoid snapshot tests, but in the end, we are testing the return of an API and we must find a balance between these two approaches.

{ x: 1598359890000, y: 0.03333333333333333 },
{ x: 1598359920000, y: 0.09333333333333334 },
{ x: 1598359950000, y: 0.014492753623188406 },
{ x: 1598359980000, y: 0.1267605633802817 },
{ x: 1598360010000, y: 0.07462686567164178 },
{ x: 1598360040000, y: 0.06578947368421052 },
{ x: 1598360070000, y: null },
],
average: 0.06805605383766485,
cauemarcondes marked this conversation as resolved.
Show resolved Hide resolved
});
});
});
});
Expand Down
Loading