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] Remove rate aggregations #112343

Closed
wants to merge 36 commits into from
Closed

[APM] Remove rate aggregations #112343

wants to merge 36 commits into from

Conversation

cauemarcondes
Copy link
Contributor

@cauemarcondes cauemarcondes commented Sep 15, 2021 •
edited
Loading

closes #112240

Remove rate aggs and calculate throughput manually.

Before:
Screen Shot 2021-09-15 at 11 17 07

After:
Screen Shot 2021-09-15 at 15 54 27

Screen Shot 2021-09-15 at 15 59 51

@cauemarcondes cauemarcondes added Team:APM All issues that need APM UI Team support release_note:skip Skip the PR/issue when compiling release notes v7.15.0 labels Sep 15, 2021
@elasticmachine
Copy link
Contributor

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

@cauemarcondes cauemarcondes requested a review from a team September 15, 2021 19:29
@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

@sorenlouv sorenlouv mentioned this pull request Sep 16, 2021
Closed
@cauemarcondes
Copy link
Contributor Author

jenkins, retest this please

@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

sorenlouv
sorenlouv requested changes Sep 30, 2021
View reviewed changes
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.

I'm a little worried about the bug I found. If it is indeed a bug it proves that we still don't have adequate test coverage.

We might want to pause this PR until we have synthetic data. This means that we need to postpone this for 7.16.

sorenlouv
sorenlouv reviewed Oct 5, 2021
View reviewed changes
x-pack/test/apm_api_integration/common/fixtures/es_archiver/archives_metadata.ts Outdated
'apm_8.0.0': {
start: '2021-08-03T06:50:15.910Z',
end: '2021-08-03T07:20:15.910Z',
},
'apm_data_generation_8.0.0': {
Copy link
Member

@sorenlouv sorenlouv Oct 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

can we find a less generic name than "apm_data_generation", eg "apm_synthetic_8.0.0"
While doing that, perhaps we can rename "apm_8.0.0" to "apm_opbeans_8.0.0"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I can change it, I'll first look into the bug you've mentioned here #112343 (comment)

sorenlouv
sorenlouv reviewed Oct 5, 2021
View reviewed changes
x-pack/test/apm_api_integration/tests/services/throughput.ts
import archives_metadata from '../../common/fixtures/es_archiver/archives_metadata';
import { FtrProviderContext } from '../../common/ftr_provider_context';
import { registry } from '../../common/registry';
import { createApmApiSupertest } from '../../common/apm_api_supertest';

type ThroughputReturn = APIReturnType<'GET /api/apm/services/{serviceName}/throughput'>;

export default function ApiTest({ getService }: FtrProviderContext) {
const apmApiSupertest = createApmApiSupertest(getService('supertest'));
Copy link
Member

Choose a reason for hiding this comment

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

This looks like the old syntax.

const apmApiClient = getService('apmApiClient');

and then

const response = await apmApiClient.readUser({...})

Copy link
Contributor Author

@cauemarcondes cauemarcondes Oct 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This branch is based on 7.15 which is why the old syntax is still being used.

Copy link
Member

Choose a reason for hiding this comment

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

ah, okay. I'm not sure it's worth backporting to 7.15 at this point tbh.

sorenlouv
sorenlouv reviewed Oct 5, 2021
View reviewed changes
x-pack/test/apm_api_integration/tests/throughput/index.ts
import { roundNumber } from '../../utils';

export default function ApiTest({ getService }: FtrProviderContext) {
const apmApiSupertest = createApmApiSupertest(getService('supertest'));
Copy link
Member

Choose a reason for hiding this comment

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

Use the new service

Suggested change
const apmApiSupertest = createApmApiSupertest(getService('supertest'));
const apmApiClient = getService('apmApiClient');

@sorenlouv
Copy link
Member

Should this be for 7.x and not 7.15?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
observability 227 228 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 4.4MB 4.4MB -554.0B
observability 494.0KB 493.9KB -79.0B
uptime 1.0MB 1.0MB +12.0B
total -621.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
apm 30 29 -1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
infra 149.8KB 149.8KB +16.0B
Unknown metric groups

API count

id before after diff
observability 227 228 +1

History

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

smith
smith reviewed Oct 6, 2021
View reviewed changes
x-pack/test/apm_api_integration/tests/services/throughput.ts
it('has the correct throughput in tpm', () => {
expectSnapshot(throughputResponse).toMatch();
expectSnapshot(throughputResponse.throughputUnit).toMatchInline(`"second"`);
it('has same mea value for both periods', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('has same mea value for both periods', () => {
it('has same mean value for both periods', () => {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorenlouv sorenlouv sorenlouv approved these changes

@smith smith smith left review comments

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v7.15.1 v7.16.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cauemarcondes @elasticmachine @dgieselaar @sorenlouv @kibanamachine @smith