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.

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

[APM] Correlations Beta (#86477) #89952

Merged
merged 23 commits into from
Feb 17, 2021
Merged

Conversation

ogupte
Copy link
Contributor

@ogupte ogupte commented Feb 2, 2021

Closes #86477.
Closes #91080.
Closes #90766.
Closes #90622.

Screen Shot 2021-02-12 at 3 15 19 AM
Screen Shot 2021-02-16 at 4 00 57 PM
Screen Shot 2021-02-16 at 3 58 49 PM

@ogupte ogupte force-pushed the apm-86477-correlations-ga branch from 82d477e to 63da7e6 Compare February 5, 2021 22:12
@ogupte ogupte marked this pull request as ready for review February 5, 2021 22:13
@ogupte ogupte requested a review from a team as a code owner February 5, 2021 22:13
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Feb 5, 2021
@elasticmachine
Copy link
Contributor

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

@ogupte ogupte added release_note:feature Makes this part of the condensed release notes v7.12.0 labels Feb 6, 2021
Comment on lines 25 to 27
const [defaultFieldNames, setDefaultFieldNames] = useState(
getDefaultFieldNames(indexPattern, isRumAgent)
);
Copy link
Member

Choose a reason for hiding this comment

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

It feels off that this is stateful. If for perf reasons you can use useMemo.
But first try to strip it down and see if it's needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's stateful because useDynamicIndexPatternFetcher is an async fetcher, so it needs to trigger a re-render when the response comes back.

 - 'show_correlations_flyout'
 - 'customize_correlations_fields'
 - 'select_significant_term'
@@ -121,7 +121,7 @@ export async function getErrorRateTimeSeries({
topSigTerms: TopSigTerm[];
}) {
const { start, end, apmEventClient } = setup;
const { intervalString } = getBucketSize({ start, end, numBuckets: 30 });
const { intervalString } = getBucketSize({ start, end, numBuckets: 15 });
Copy link
Member

@sorenlouv sorenlouv Feb 9, 2021

Choose a reason for hiding this comment

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

I think this will still return a dynamic number of buckets to aim for a "nice" bucket size.
Some feedback I got was to make the number of buckets fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll fix this in a separate issue: #91574

Comment on lines 112 to 115
const buckets = dropRightWhile(
distribution.dist_filtered_by_latency.buckets,
(bucket, index) => bucket.doc_count === 0 && index > intervalBuckets - 1
);
Copy link
Member

@sorenlouv sorenlouv Feb 9, 2021

Choose a reason for hiding this comment

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

I don't understand why we are doing this. If we want fewer bucket returned shouldn't we specify a larger interval?

Copy link
Member

@sorenlouv sorenlouv Feb 9, 2021

Choose a reason for hiding this comment

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

Btw. all this should matter less if we use a logarithmic scale. Did we decide to punt on that?

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 created a separate issue for the distribution scale: #91574

@ogupte ogupte requested review from a team as code owners February 16, 2021 17:31
@ogupte
Copy link
Contributor Author

ogupte commented Feb 16, 2021

jenkins run the e2e

@@ -4384,9 +4384,6 @@
},
"apm:enableServiceOverview": {
"type": "boolean"
},
"apm:enableCorrelations": {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, You'd need to open a corresponding mapping update request in the infra repo to have this key removed from the telemetry mappings.
If you don't care that it's not there in versions from 7.12, then you won't need to open an issue for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll probably remove it from the mapping in the infra repo in 7.13. That way we'll be able to tell if users are still using it as a custom UI Setting for version 7.12.

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

I reviewed both the ui_settings key removal and the usage collection schema update for telemetry.
LGTM

@ogupte ogupte changed the title [APM] Correlations GA (#86477) [APM] Correlations Beta (#86477) Feb 16, 2021
@ogupte
Copy link
Contributor Author

ogupte commented Feb 17, 2021

jenkins run the e2e

{
range: {
[TRANSACTION_DURATION]: { gte: durationForPercentile },
filter: backgroundFilters,
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for removing the range query for duration? Will removing it not hurt perf since more documents are retrieved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moving the range query to the function_score provided us with a score that we needed to order foreground results that were most important to us (those with higher durations)

Comment on lines +105 to +107
[TRANSACTION_DURATION]: {
lt: durationForPercentile,
},
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the foreground supposed to be a subset of the background?

Copy link
Member

Choose a reason for hiding this comment

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

... if not you may have to set this

"background_is_superset": false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We found that the existing background filter included too much uninteresting data and it was skewing the results of significant terms agg to be not as helpful see(https://youtu.be/azP15yvbOBA)

Comment on lines -39 to +58
bgCount: bucket.bg_count,
fgCount: bucket.doc_count,
fieldCount: agg.doc_count,
valueCount: bucket.doc_count,
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this change. Can you elaborate a bit?

Copy link
Contributor Author

@ogupte ogupte Feb 17, 2021

Choose a reason for hiding this comment

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

the previous bgCount wasn't needed since it represents only values for that field/value pair, when we really wanted to express the percentage in terms of the total number of background documents the aggregation applies to. E.g. user_agent.name: HeadlessChrome at the 75th percentile threshold accounts for only 8% of traffic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

previously it showed something like "80% of documents with user_agent.name: HeadlessChrome exist in the 75th percentile threshold", so it's a question of what is easier to understand for the user.

@ogupte
Copy link
Contributor Author

ogupte commented Feb 17, 2021

jenkins run the e2e

1 similar comment
@ogupte
Copy link
Contributor Author

ogupte commented Feb 17, 2021

jenkins run the e2e

@ogupte
Copy link
Contributor Author

ogupte commented Feb 17, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1731 1733 +2

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.2MB 5.2MB +9.9KB

History

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

@ogupte ogupte merged commit d7d2b15 into elastic:master Feb 17, 2021
ogupte added a commit to ogupte/kibana that referenced this pull request Feb 17, 2021
* [APM] Correlations GA (elastic#86477)

* polish and improvements to correlations UI

* more improvements and polish

* added impact bar

* added descriptions

* make custom field persistence be unique per service

* make custom threshold unique per service in latency correlations

* adds telemetry for apm correlations feature. Events:
 - 'show_correlations_flyout'
 - 'customize_correlations_fields'
 - 'select_significant_term'

* adds more telemetry for correlations (elastic#90622)

* removes the raw score column

* replaces experiemental callout with beta badge

* replaces threshold number input with percentile option selector

* improvements to latency correlations scoring and percentage reporting

* removes the 'apm:enableCorrelations' UI setting

* - rename useFieldNames.ts -> use_field_names.ts
- filter out fields that are not type 'keyword'
- feedback improvements

* Fixes casing issue for the 'correlations' dir

* [APM] Moves correlations button to service details tabslist row (elastic#91080)

* [APM] Adds license check for correlations (elastic#90766)

* [APM] Adds metrics tracking for correlations views and license prompts (elastic#90622)

* Updated the API integration tests to check for new default fields and 15 buckets

Co-authored-by: Kibana Machine <[email protected]>
ogupte added a commit that referenced this pull request Feb 17, 2021
* [APM] Correlations GA (#86477)

* polish and improvements to correlations UI

* more improvements and polish

* added impact bar

* added descriptions

* make custom field persistence be unique per service

* make custom threshold unique per service in latency correlations

* adds telemetry for apm correlations feature. Events:
 - 'show_correlations_flyout'
 - 'customize_correlations_fields'
 - 'select_significant_term'

* adds more telemetry for correlations (#90622)

* removes the raw score column

* replaces experiemental callout with beta badge

* replaces threshold number input with percentile option selector

* improvements to latency correlations scoring and percentage reporting

* removes the 'apm:enableCorrelations' UI setting

* - rename useFieldNames.ts -> use_field_names.ts
- filter out fields that are not type 'keyword'
- feedback improvements

* Fixes casing issue for the 'correlations' dir

* [APM] Moves correlations button to service details tabslist row (#91080)

* [APM] Adds license check for correlations (#90766)

* [APM] Adds metrics tracking for correlations views and license prompts (#90622)

* Updated the API integration tests to check for new default fields and 15 buckets

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 17, 2021
* master: (157 commits)
  [DOCS] Adds machine learning to the security section of alerting (elastic#91501)
  [Uptime] Ping list step screenshot caption formatting (elastic#91403)
  [Vislib] Use timestamp on brush event instead of iso dates (elastic#91483)
  [Application Usage] Remove deprecated & unused legacy.appChanged API (elastic#91464)
  Migrate logstash, monitoring, url_drilldowns, xpack_legacy to ts projects (elastic#91194)
  [APM] Wrap Elasticsearch client errors (elastic#91125)
  [APM] Fix optimize-tsconfig script (elastic#91487)
  [Discover][docs] Add searchFieldsFromSource description (elastic#90980)
  Adds support for 'ip' data type (elastic#85087)
  [Detection Rules] Add updates from 7.11.2 rules (elastic#91553)
  [SECURITY SOLUTION] Eql in timeline (elastic#90816)
  [APM] Correlations Beta (elastic#86477) (elastic#89952)
  [Security Solutions][Detection Engine] Adds a warning banner when the alerts data has not been migrated yet. (elastic#90258)
  [Security Solution] [Timeline] Endpoint row renderers (2nd batch) (elastic#91446)
  skip flaky suite (elastic#91450)
  skip flaky suite (elastic#91592)
  [Security Solution][Endpoint][Admin] Endpoint Details UX Enhancements (elastic#90870)
  [ML] Add better UI support for runtime fields Transforms  (elastic#90363)
  [Security Solution] [Detections] Replace 'partial failure' with 'warning' for rule statuses (elastic#91167)
  [Security Solution][Detections] Adds Indicator path config for indicator match rules (elastic#91260)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:feature Makes this part of the condensed release notes Team:APM All issues that need APM UI Team support v7.12.0
Projects
None yet
5 participants