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] Always get the root transaction when fetching trace items #42508

Conversation

@ogupte
Copy link
Contributor

commented Aug 2, 2019

Fixes #33210

The bug was due to the root transaction not being in the response. This fix always ensures that the root transaction exists in the response.

Test by using the hey-apm tool to create a transaction with 10000 spans:
hey-apm -t 1 -e 0 -sm 10000 -sx 10000

Added messaging to notify user that the number of items in the trace exceed what is being displayed in the waterfall:
Screen Shot 2019-08-05 at 12 06 13 AM

@ogupte ogupte requested a review from elastic/apm-ui as a code owner Aug 2, 2019

@ogupte ogupte self-assigned this Aug 2, 2019

const root: Transaction | undefined = idx(resp, _ => _.hits.hits[0]._source);

return root;
}

This comment has been minimized.

Copy link
@sqren

sqren Aug 2, 2019

Member

Instead of fetching the root transaction separately, I think we can add a should clause to the existing getTraceItems so that the root transaction will have higher score, and thus will be the very first item in the list:

should: {
  exists: { field: PARENT_ID }
}

Additionally we should sort by duration to get the longest transactions/spans since they are often in the beginning:

"sort": [
  { "_score": { "order": "desc" }},
  { "transaction.duration.us":   { "order": "desc" }}, // this could be a little tricky since span docs don't have this field
  { "span.duration.us":   { "order": "desc" }} // this could be a little tricky since transaction docs don't have this field
]

And finally I think we should also show a warning message if there are items that are not being shown.

This will not solve the problem with max 1000 items but it'll hopefully make it a bit better until we can make the "recursive loading" solution we've talked about

This comment has been minimized.

Copy link
@sqren

sqren Aug 5, 2019

Member

I can see that you've added some commits to this PR. It's still the intention to get rid of this call (getTraceRoot), right?

This comment has been minimized.

Copy link
@ogupte

ogupte Aug 5, 2019

Author Contributor

After some initial difficulty, @dgieselaar and I found right right combination of should clause and sort order to always return the root transaction in the same query in order to remove getTraceRoot.

@elasticmachine

This comment has been minimized.

Copy link

commented Aug 2, 2019

@ogupte ogupte force-pushed the ogupte:apm-33210-fix-trace-waterfall-with-over-1000-items branch from 5a299b3 to 2e2867d Aug 5, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented Aug 5, 2019

} from '../../../common/elasticsearch_fieldnames';
import { Span } from '../../../typings/es_schemas/ui/Span';
import { Transaction } from '../../../typings/es_schemas/ui/Transaction';
import { rangeFilter } from '../helpers/range_filter';
import { Setup } from '../helpers/setup_request';

const MAX_ITEMS_RETURNED = 1000;

This comment has been minimized.

Copy link
@sqren

sqren Aug 5, 2019

Member

Can you make this configurable via kibana.yml?

This comment has been minimized.

Copy link
@ogupte

ogupte Aug 5, 2019

Author Contributor

created apm_oss.maxTraceItems to control this value.

@ogupte ogupte force-pushed the ogupte:apm-33210-fix-trace-waterfall-with-over-1000-items branch from 2e2867d to 54c4c23 Aug 5, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented Aug 5, 2019

apmAgentConfigurationIndex: Joi.string().default('.apm-agent-configuration'),

// UI settings
maxTraceItems: Joi.number().default(1000)

This comment has been minimized.

Copy link
@sqren

sqren Aug 5, 2019

Member

I apologize in advance because this is super confusing but since this setting is only relevant for APM UI it should be in the xpack/apm package.
The only reason we created the apm_oss package was so users can still use the APM dashboards (and check for data via the getting started guide) even if they have APM UI disabled.

return resp.hits.hits.map(hit => hit._source);
return {
items: resp.hits.hits.map(hit => hit._source),
exceedsMax: resp.hits.total > maxTraceItems

This comment has been minimized.

Copy link
@sqren

sqren Aug 5, 2019

Member

Note: From version 7.0 elasticsearch will not produce accurate counts above 10,000 docs. We added a workaround for this with rest_total_hits_as_int so accurate counts are always returned for now but we'll remove that in #34915. We just have to remember to update this accordingly when we get around to that.

@@ -246,7 +246,7 @@ export function getWaterfall(
};
}

const waterfallItems = trace.map(traceItem => {
const waterfallItems = [...trace.items].map(traceItem => {

This comment has been minimized.

Copy link
@sqren

sqren Aug 5, 2019

Member

Any reason for not doing this?

Suggested change
const waterfallItems = [...trace.items].map(traceItem => {
const waterfallItems = trace.items.map(traceItem => {

This comment has been minimized.

Copy link
@ogupte

ogupte Aug 5, 2019

Author Contributor

whoops, i forgot the change this back, when i removed the root fetch

iconType="alert"
title={i18n.translate('xpack.apm.waterfall.exceedsMax', {
defaultMessage:
'Number of items in this trace exceed what is displayed'

This comment has been minimized.

Copy link
@sqren

sqren Aug 5, 2019

Member

I think this is a little cryptic if you don't know what we are talking about. Ideally we'd let the user know how many trace items are being left out like:
"Only the first 1000 items in a trace will be displayed, and 293 items were therefore left out ". Although having those two variables (1000 and 293) will be a bit of a hassle so I'd rather not do that.

Perhaps @bmorelli25 has an idea on how to improve this.

This comment has been minimized.

Copy link
@bmorelli25

bmorelli25 Aug 5, 2019

Member

Hmmm, I'm struggling on this one. Without using variables I'm not sure we can do much better here.

This comment has been minimized.

Copy link
@ogupte

ogupte Aug 6, 2019

Author Contributor

Variables aren't completely off the table for messaging here, but it does require some extra data being passed to support it. If the only useful messaging requires these variables, I can make it work.

This comment has been minimized.

Copy link
@sqren

sqren Aug 6, 2019

Member

I'm good with how it is now if @bmorelli25 is :)

This comment has been minimized.

Copy link
@bmorelli25

bmorelli25 Aug 6, 2019

Member

I'm good with it as well. We can always improve it later if need be.

@elasticmachine

This comment has been minimized.

Copy link

commented Aug 6, 2019

@@ -38,7 +38,7 @@ export default function apmOss(kibana) {
spanIndices: Joi.string().default('apm-*'),
metricsIndices: Joi.string().default('apm-*'),
onboardingIndices: Joi.string().default('apm-*'),
apmAgentConfigurationIndex: Joi.string().default('.apm-agent-configuration')
apmAgentConfigurationIndex: Joi.string().default('.apm-agent-configuration'),

This comment has been minimized.

Copy link
@sqren

sqren Aug 6, 2019

Member

I wonder if this should have been in the OSS version. Probably not.

This comment has been minimized.

Copy link
@sqren

sqren Aug 6, 2019

Member

Btw. now you are at it, can you add this to the docs like you did with maxTraceItems?

@@ -21,6 +21,8 @@ xpack.apm.ui.enabled:: Set to `false` to hide the APM plugin {kib} from the menu

xpack.apm.ui.transactionGroupBucketSize:: Number of top transaction groups displayed in APM plugin in Kibana. Defaults to `100`.

xpack.apm.ui.maxTraceItems:: Max number of child items displayed when viewing trace details. Defaults to `1000`.

This comment has been minimized.

Copy link
@sqren

sqren Aug 6, 2019

Member

It used to be required to add new options to the docker image (or they wouldn't be available for users using the official Kibana Docker image).
Here' s an example of some settings I added to the whitelist: elastic/kibana-docker#129

However, that repo has been archived and it's been merged with the normal kibana repo afaik. Would be good to find out if this step is still required.

@sqren

sqren approved these changes Aug 6, 2019

Copy link
Member

left a comment

LGTM!

@ogupte ogupte requested a review from elastic/kibana-operations as a code owner Aug 6, 2019

apm_oss.onboardingIndices
apm_oss.spanIndices
apm_oss.transactionIndices
apm_oss.metricsIndices

This comment has been minimized.

Copy link
@ogupte

ogupte Aug 6, 2019

Author Contributor

@sqren i added these other configs to the docker template since i noticed they were missing. maybe they weren't copied over from the old docker template

This comment has been minimized.

Copy link
@sqren

sqren Aug 6, 2019

Member

Awesome! Good find 👍

@jbudz

jbudz approved these changes Aug 6, 2019

Copy link
Contributor

left a comment

kibana-docker LGTM

@sqren

sqren approved these changes Aug 6, 2019

Copy link
Member

left a comment

LGTM

@elasticmachine

This comment has been minimized.

Copy link

commented Aug 6, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented Aug 6, 2019

@ogupte

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

retest

@elasticmachine

This comment has been minimized.

Copy link

commented Aug 6, 2019

@ogupte ogupte force-pushed the ogupte:apm-33210-fix-trace-waterfall-with-over-1000-items branch from 231449a to 2c7e019 Aug 7, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented Aug 7, 2019

@ogupte ogupte merged commit 8b57570 into elastic:master Aug 7, 2019

50 checks passed

API integration tests node scripts/functional_tests --config test/api_integration/config.js --bail --debug
Details
Browser tests yarn run grunt test:browser-ci
Details
Build kbn_tp_sample_panel_action yarn build
Details
CLA All commits in pull request signed
Details
Check core API changes node scripts/check_core_api_changes
Details
Check file casing node scripts/check_file_casing --quiet
Details
Check licenses node scripts/check_licenses --dev
Details
Firefox smoke test node scripts/functional_tests --bail --debug --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/kibana-firefoxSmoke/node/linux-immutable/install/kibana --include-tag smoke --config test/functional/config.firefox.js
Details
Functional tests / Group 1 yarn run grunt run:functionalTests_ciGroup1
Details
Functional tests / Group 10 yarn run grunt run:functionalTests_ciGroup10
Details
Functional tests / Group 11 yarn run grunt run:functionalTests_ciGroup11
Details
Functional tests / Group 12 yarn run grunt run:functionalTests_ciGroup12
Details
Functional tests / Group 2 yarn run grunt run:functionalTests_ciGroup2
Details
Functional tests / Group 3 yarn run grunt run:functionalTests_ciGroup3
Details
Functional tests / Group 4 yarn run grunt run:functionalTests_ciGroup4
Details
Functional tests / Group 5 yarn run grunt run:functionalTests_ciGroup5
Details
Functional tests / Group 6 yarn run grunt run:functionalTests_ciGroup6
Details
Functional tests / Group 7 yarn run grunt run:functionalTests_ciGroup7
Details
Functional tests / Group 8 yarn run grunt run:functionalTests_ciGroup8
Details
Functional tests / Group 9 yarn run grunt run:functionalTests_ciGroup9
Details
Internationalization check node scripts/i18n_check --ignore-missing
Details
Interpreter functional tests node scripts/functional_tests --config test/interpreter_functional/config.js --bail --debug --kibana-install-dir ./build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64
Details
Jest integration tests yarn run grunt test:jest_integration
Details
Jest tests yarn run grunt test:jest
Details
Mocha tests node scripts/mocha
Details
Plugin functional tests node scripts/functional_tests --config test/plugin_functional/config.js --bail --debug --kibana-install-dir ./build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64
Details
Project tests yarn run grunt test:projects
Details
Type check node scripts/type_check
Details
TypeScript - all files belong to a TypeScript project node scripts/check_ts_projects
Details
Verify NOTICE.txt node scripts/notice --validate
Details
Verify dependency versions yarn run grunt verifyDependencyVersions
Details
X-Pack Chrome Functional tests / Group 1 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup1/node/linux-immutable/install/kibana --include-tag ciGroup1
Details
X-Pack Chrome Functional tests / Group 10 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup10/node/linux-immutable/install/kibana --include-tag ciGroup10
Details
X-Pack Chrome Functional tests / Group 2 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup2/node/linux-immutable/install/kibana --include-tag ciGroup2
Details
X-Pack Chrome Functional tests / Group 3 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup3/node/linux-immutable/install/kibana --include-tag ciGroup3
Details
X-Pack Chrome Functional tests / Group 4 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup4/node/linux-immutable/install/kibana --include-tag ciGroup4
Details
X-Pack Chrome Functional tests / Group 5 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup5/node/linux-immutable/install/kibana --include-tag ciGroup5
Details
X-Pack Chrome Functional tests / Group 6 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup6/node/linux-immutable/install/kibana --include-tag ciGroup6
Details
X-Pack Chrome Functional tests / Group 7 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup7/node/linux-immutable/install/kibana --include-tag ciGroup7
Details
X-Pack Chrome Functional tests / Group 8 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup8/node/linux-immutable/install/kibana --include-tag ciGroup8
Details
X-Pack Chrome Functional tests / Group 9 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup9/node/linux-immutable/install/kibana --include-tag ciGroup9
Details
X-Pack Jest node scripts/jest --ci --verbose
Details
X-Pack Mocha yarn test
Details
X-Pack SIEM cyclic dependency test node legacy/plugins/siem/scripts/check_circular_deps
Details
X-Pack firefox smoke test node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-firefoxSmoke/node/linux-immutable/install/kibana --include-tag smoke --config test/functional/config.firefox.js
Details
eslint node scripts/eslint --no-cache
Details
kibana-ci Build finished.
Details
prbot:outdated run `node scripts/update_prs 42508` to update
prbot:release note labels
sasslint node scripts/sasslint
Details

ogupte added a commit to ogupte/kibana that referenced this pull request Aug 7, 2019

[APM] Always get the root transaction when fetching trace items (elas…
…tic#42508)

* [APM] Always get the root transaction when fetching trace items
Fixes elastic#33210

* code tweaks

* displays message notifying user that trace items exceeds maximum displayed

* remove getTraceRoot query by adjusting the score and order of trace
items with no parent.id

* add `apm_oss.maxTraceItems` config options to control the number of displayed trace items

* changed config `apm_oss.maxTraceItems` to `xpack.apm.ui.maxTraceItems`

* added missing configs to apm settings doc and docker template

* minor code tweak

ogupte added a commit that referenced this pull request Aug 7, 2019

[APM] Always get the root transaction when fetching trace items (#42508
…) (#42812)

* [APM] Always get the root transaction when fetching trace items
Fixes #33210

* code tweaks

* displays message notifying user that trace items exceeds maximum displayed

* remove getTraceRoot query by adjusting the score and order of trace
items with no parent.id

* add `apm_oss.maxTraceItems` config options to control the number of displayed trace items

* changed config `apm_oss.maxTraceItems` to `xpack.apm.ui.maxTraceItems`

* added missing configs to apm settings doc and docker template

* minor code tweak
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.