-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ML] APM Latency Correlations: Field/value candidates prioritization #107370
[ML] APM Latency Correlations: Field/value candidates prioritization #107370
Conversation
201dd65
to
3964729
Compare
6c0a11f
to
78362e3
Compare
Pinging @elastic/ml-ui (:ml) |
Pinging @elastic/apm-ui (Team:apm) |
if ( | ||
params === undefined || | ||
item === undefined || | ||
state.getState().isCancelled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is used a few times inside this for loop it might make sense to assign it to a variable outside the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The process could have been cancelled by the user in parallel while the for loop is still running, that's why we always need to check for the latest state of isCancelled
, if we assign it once outside the loop it could be out of date. I added a helper getter so we can use state.getIsCancelled()
in af7afce to simplify the usage a bit.
x-pack/plugins/apm/server/lib/search_strategies/correlations/queries/query_field_value_pairs.ts
Outdated
Show resolved
Hide resolved
...s/apm/server/lib/search_strategies/correlations/queries/get_prioritized_field_value_pairs.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/server/lib/search_strategies/correlations/queries/get_request_base.test.ts
Show resolved
Hide resolved
LGTM, just left a few comments 🎉 Tested the overall behavior and everything looks good, will look into testing the new |
@@ -48,17 +48,17 @@ export const fetchTransactionDurationFieldValuePairs = async ( | |||
esClient: ElasticsearchClient, | |||
params: SearchServiceFetchParams, | |||
fieldCandidates: Field[], | |||
progress: AsyncSearchProviderProgress | |||
state: AsyncSearchServiceState | |||
): Promise<FieldValuePairs> => { | |||
const fieldValuePairs: FieldValuePairs = []; | |||
|
|||
let fieldValuePairsProgress = 1; | |||
|
|||
for (let i = 0; i < fieldCandidates.length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this would become easier to read if you create a utility function that makes it explicit that we want to iterate sequentially through the array (as opposed to doing it in parallel with arr.map
):
async function runInSequence(items, fn) {
const results = [];
for (const item of items) {
results.push(await fn(item));
}
return results;
}
And using it like:
const fieldValuePairs = runInSequence(fieldCandidates, async (fieldName) => {
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, refactored in dd7636c.
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Chrome UI Functional Tests.test/functional/apps/management/_index_pattern_create_delete·js.management creating and deleting default index validation can display errorsStandard Out
Stack Trace
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @walterra |
@sqren this is ready for another look |
…lastic#107370) - Makes sure fields defined in `FIELDS_TO_ADD_AS_CANDIDATE` and prefixed with one of `FIELD_PREFIX_TO_ADD_AS_CANDIDATE` get queried first when retrieving the `correlation` and `ks-test` value. - Correctly consider the `includeFrozen` parameter. - The bulk of the PR is a refactor: - Moves `query_*` files to `queries` directory - Introduces `asyncSearchServiceStateProvider` to manage the state of the async search service in isolation so that we no longer mutate individual vars or plain objects. - Introduces `asyncSearchServiceLogProvider` and extends the log to not only store messages but original error messages retrieved from ES too. - Refactors some more functions in separate files and adds unit tests. - Removes some deprecated code no longer needed.
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…-png-pdf-report-type * 'master' of github.com:elastic/kibana: (101 commits) [ML] APM Latency Correlations: Field/value candidates prioritization (elastic#107370) [Reporting] Add lenience to a test on the order of asserted logs (elastic#108135) [Lens] fix do not submit invalid query in filtered metric (elastic#107542) skip flaky test (elastic#108043) fix newly introduced type error (elastic#107593) [Reporting] server side code clean up (elastic#106940) [build_ts_refs] improve caches, allow building a subset of projects (elastic#107981) [APM] Add new ftr_e2e to kibana CI and remove current e2e tests. (elastic#107593) add manage rules link to alerts dropdown (elastic#107950) [ML] Enable Index data visualizer document count chart to update time range query (elastic#106438) [Security Solutions][Detection Engine] Fixes "undefined" crash for author field by adding a migration for it (elastic#107230) [Actions UI] Fixed Jira Api token label. (elastic#107776) [Alerting UI] Fixed display permissions for edit/delete buttons when user has read only access. (elastic#107996) [Maps] fix code owners (elastic#108106) Update EMS landing page url (elastic#108102) Do not render page header for loading domains (elastic#108078) Update dependency @elastic/charts to v33.2.2 (elastic#107939) [APM] Display throughput as tps (instead of tpm) when bucket size < 60 seconds (elastic#107850) [Fleet] Fix all category count (elastic#108089) [Security Solution][Bug] - Disable alert table RBAC until fields sorted (elastic#108034) ... # Conflicts: # x-pack/plugins/reporting/server/export_types/common/generate_png.ts # x-pack/plugins/reporting/server/lib/screenshots/index.ts # x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts # x-pack/plugins/reporting/server/lib/screenshots/observable.ts
…107370) (#108152) - Makes sure fields defined in `FIELDS_TO_ADD_AS_CANDIDATE` and prefixed with one of `FIELD_PREFIX_TO_ADD_AS_CANDIDATE` get queried first when retrieving the `correlation` and `ks-test` value. - Correctly consider the `includeFrozen` parameter. - The bulk of the PR is a refactor: - Moves `query_*` files to `queries` directory - Introduces `asyncSearchServiceStateProvider` to manage the state of the async search service in isolation so that we no longer mutate individual vars or plain objects. - Introduces `asyncSearchServiceLogProvider` and extends the log to not only store messages but original error messages retrieved from ES too. - Refactors some more functions in separate files and adds unit tests. - Removes some deprecated code no longer needed. Co-authored-by: Walter Rafelsberger <[email protected]>
Summary
Part of #106381
FIELDS_TO_ADD_AS_CANDIDATE
and prefixed with one ofFIELD_PREFIX_TO_ADD_AS_CANDIDATE
get queried first when retrieving thecorrelation
andks-test
value.includeFrozen
parameter.query_*
files toqueries
directoryasyncSearchServiceStateProvider
to manage the state of the async search service in isolation so that we no longer mutate individual vars or plain objects.asyncSearchServiceLogProvider
and extends the log to not only store messages but original error messages retrieved from ES too.Checklist