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] Correlations polish #85116

Merged
merged 8 commits into from
Dec 15, 2020
Merged

[APM] Correlations polish #85116

merged 8 commits into from
Dec 15, 2020

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Dec 7, 2020 •
edited
Loading

Closes #84863

image

image

cc @formgeist

@sorenlouv sorenlouv marked this pull request as ready for review December 7, 2020 14:57
@sorenlouv sorenlouv requested a review from a team as a code owner December 7, 2020 14:57
@sorenlouv sorenlouv added v7.11.0 release_note:skip Skip the PR/issue when compiling release notes labels Dec 7, 2020
@botelastic botelastic bot added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Dec 8, 2020
@elasticmachine
Copy link
Contributor

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

@sorenlouv sorenlouv force-pushed the correlations-improvements branch from d6496a4 to 06f4be9 Compare December 8, 2020 23:29
formgeist
formgeist approved these changes Dec 9, 2020
View reviewed changes
Copy link
Contributor

@formgeist formgeist left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Seeing as this is an experimental feature, I'd rather just get these fixes in. I'm a bit puzzled by the addition of the customize feature (which makes it harder to pull it out once added). @nehaduggal do you have any feedback on the Customize functionality?

Additionally, shouldn't service.version be one of the default fields that we search for?

@formgeist formgeist requested a review from a team December 9, 2020 08:38
@sorenlouv
Copy link
Member Author

sorenlouv commented Dec 9, 2020 •
edited
Loading

I'm a bit puzzled by the addition of the customize feature (which makes it harder to pull it out once added). @nehaduggal do you have any feedback on the Customize functionality?

Right now there is a hardcoded list of fields. That doesn't work well (especially for user defined fields like labels), so we need a way for users to specify which fields they want analysed. This was the simplest approach I could think of. I'm open to other approaches.

I don't think it's hard to pull out. This is an experimental feature - we can do anything we want. Even pull the entire feature and never ship it.

smith
smith approved these changes Dec 10, 2020
View reviewed changes
x-pack/plugins/apm/public/components/app/Correlations/LatencyCorrelations.tsx
<EuiFlexItem>
<SignificantTermsTable
cardinalityColumnName="# of slow transactions"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we adding i18n at this stage?

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 thought about it and opted not to. There's a good chance that all of the copy will be changed anyway (I made all of it up) - so translators will have to do this twice. Not sure what the official guidelines around experimental features are.

@sorenlouv
Copy link
Member Author

@elasticmachine merge upstream

@sorenlouv
Copy link
Member Author

jenkins run the e2e

1 similar comment
@sorenlouv
Copy link
Member Author

jenkins run the e2e

@sorenlouv
Copy link
Member Author

@elasticmachine merge upstream

@sorenlouv
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
apm 5.4MB 5.4MB +1.1KB

Distributable file count

id before after diff
default 47130 47890 +760

History

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

@sorenlouv
Copy link
Member Author

jenkins run the e2e

@sorenlouv sorenlouv merged commit 20638a6 into elastic:master Dec 15, 2020
@sorenlouv sorenlouv deleted the correlations-improvements branch December 15, 2020 12:15
@sorenlouv sorenlouv mentioned this pull request Dec 15, 2020
Merged
sorenlouv added a commit that referenced this pull request Dec 16, 2020
@sorenlouv @kibanamachine
[7.x] [APM] Correlations polish (#85116) (#85940)
42b3ecb 
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smith smith smith approved these changes

@formgeist formgeist formgeist approved these changes

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:APM - DEPRECATED Use Team:obs-ux-infra_services. v7.11.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM] Design polish: Significant tags
5 participants
@sorenlouv @elasticmachine @kibanamachine @smith @formgeist