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

[data views] More efficient telemetry collection #152298

Merged
merged 14 commits into from
Mar 20, 2023

Conversation

mattkime
Copy link
Contributor

@mattkime mattkime commented Feb 27, 2023

Summary

Telemetry no longer makes field caps calls. Also batch loads data view saved objects.

To collect telemetry -

POST kbn:/api/telemetry/v2/clusters/_stats
{
}

Addresses: https://github.com/elastic/sdh-kibana/issues/3389

Checklist

@mattkime mattkime changed the title more efficient data view telemetry [data views] More efficient telemetry collection Feb 27, 2023
results.runtimeFieldCount / results.indexPatternsWithRuntimeFieldCount;
}
});
}

return results;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

console.log here to see results. results are encrypted in the http response. You may need to restart kibana to run this multiple times.

@mattkime mattkime added Feature:Data Views Data Views code and UI - index patterns before 8.0 Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Feb 27, 2023
@mattkime mattkime marked this pull request as ready for review February 27, 2023 23:15
@mattkime mattkime requested a review from a team as a code owner February 27, 2023 23:15
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Looks like this will be huge improvement in terms efficiency 👍
Tried it with my local setup, I was just testing data views with no indices. it threw an exception
Bildschirmfoto 2023-02-28 um 14 43 56

results.indexPatternsWithRuntimeFieldCount++;
results.runtimeFieldCount += runtimeFields.length;
savedObjects.forEach((obj) => {
const fields = JSON.parse(obj.attributes.fields) || [];
Copy link
Member

Choose a reason for hiding this comment

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

the JSON.parse threw an exception locally, so it's recommendable to use atry{}catch(){} pattern for this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I had failed to push a change. I also changed the default data view definition in the test not to have the attributes that are used as to provide test coverage.

savedObjects.forEach((obj) => {
const fields = JSON.parse(obj.attributes.fields) || [];
const runtimeFieldsMap = obj.attributes.runtimeFieldMap
? JSON.parse(obj.attributes.runtimeFieldMap) || {}
Copy link
Member

Choose a reason for hiding this comment

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

This is also a candidate for try - catch

Copy link
Member

Choose a reason for hiding this comment

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

@mattkime Telemetry stats now work as excepted 👍 ... one think I still think worth to consider is using a try - catch pattern for the JSON.parse. Because if for whatever reason an invalid JSON string is passed in, a SyntaxError is thrown, and this would lead and the one hostile data view would stop the generation of the results ... or do I miss somewhere this was caught? ... or would such a case also lead to troubles before this change?


const scriptedFields = ip.getScriptedFields();
const runtimeFields = ip.fields.filter((fld) => !!fld.runtimeField);
const finder = savedObjectsService.createPointInTimeFinder<DataViewFieldAttrs>(findOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also call finder.close() or it's not required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its not needed with the for loop.

@rudolf
Copy link
Contributor

rudolf commented Mar 1, 2023

It'll be easier if we could get that merged into main, but would be good to test this with #151110

@mattkime mattkime requested a review from kertal March 1, 2023 11:32
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Tested it and I found that when using our demo data views, the result of the telemetry is different

Before:

{
indexPatternsCount: 3,
indexPatternsWithScriptedFieldCount: 0,
indexPatternsWithRuntimeFieldCount: 2,
scriptedFieldCount: 0,
runtimeFieldCount: 2,
perIndexPattern: {
scriptedFieldCount: { min: undefined, max: undefined, avg: undefined },
runtimeFieldCount: { min: 1, max: 1, avg: 1 },
scriptedFieldLineCount: { min: undefined, max: undefined, avg: undefined },
runtimeFieldLineCount: { min: 1, max: 1, avg: 1 }
}
}

After

{
indexPatternsCount: 3,
indexPatternsWithScriptedFieldCount: 0,
indexPatternsWithRuntimeFieldCount: 0,
scriptedFieldCount: 0,
runtimeFieldCount: 0,
perIndexPattern: {
scriptedFieldCount: { min: undefined, max: undefined, avg: undefined },
runtimeFieldCount: { min: undefined, max: undefined, avg: undefined },
scriptedFieldLineCount: { min: undefined, max: undefined, avg: undefined },
runtimeFieldLineCount: { min: undefined, max: undefined, avg: undefined }
}
}

so the field count doesn't seem to work

@mattkime
Copy link
Contributor Author

mattkime commented Mar 1, 2023

@kertal fixed. I was requesting the fields wrong, its 'runtimeFieldMap' not 'attributes.runtimeFieldMap'

@kertal kertal self-requested a review March 2, 2023 11:40
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #18 / security APIs - Session Idle Session POST /internal/security/session should extend the session

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 433 436 +3

References to deprecated APIs

id before after diff
dataViews 150 148 -2

Total ESLint disabled count

id before after diff
securitySolution 513 516 +3

History

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

cc @mattkime

@mattkime
Copy link
Contributor Author

@kertal I think I still need an official 👍

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

LGTM, tested the payload sent for telemetry before and with this PR using our demo datasets. it's identical 👍

@mattkime mattkime merged commit 1892bf6 into elastic:main Mar 20, 2023
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 20, 2023
## Summary

Telemetry no longer makes field caps calls. Also batch loads data view
saved objects.

To collect telemetry -
```
POST kbn:/api/telemetry/v2/clusters/_stats
{
}
```

Addresses: elastic/sdh-kibana#3389

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

(cherry picked from commit 1892bf6)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.7

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Mar 20, 2023
…3293)

# Backport

This will backport the following commits from `main` to `8.7`:
- [[data views] More efficient telemetry collection
(#152298)](#152298)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Matthew
Kime","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-03-20T12:52:55Z","message":"[data
views] More efficient telemetry collection (#152298)\n\n##
Summary\r\n\r\nTelemetry no longer makes field caps calls. Also batch
loads data view\r\nsaved objects.\r\n\r\nTo collect telemetry -
\r\n```\r\nPOST
kbn:/api/telemetry/v2/clusters/_stats\r\n{\r\n}\r\n```\r\n\r\nAddresses:
https://github.com/elastic/sdh-kibana/issues/3389\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"1892bf65352aa819cefb0408c507ad894edb8a16","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Data
Views","release_note:skip","Team:DataDiscovery","backport:prev-minor","v8.8.0"],"number":152298,"url":"https://github.com/elastic/kibana/pull/152298","mergeCommit":{"message":"[data
views] More efficient telemetry collection (#152298)\n\n##
Summary\r\n\r\nTelemetry no longer makes field caps calls. Also batch
loads data view\r\nsaved objects.\r\n\r\nTo collect telemetry -
\r\n```\r\nPOST
kbn:/api/telemetry/v2/clusters/_stats\r\n{\r\n}\r\n```\r\n\r\nAddresses:
https://github.com/elastic/sdh-kibana/issues/3389\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"1892bf65352aa819cefb0408c507ad894edb8a16"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/152298","number":152298,"mergeCommit":{"message":"[data
views] More efficient telemetry collection (#152298)\n\n##
Summary\r\n\r\nTelemetry no longer makes field caps calls. Also batch
loads data view\r\nsaved objects.\r\n\r\nTo collect telemetry -
\r\n```\r\nPOST
kbn:/api/telemetry/v2/clusters/_stats\r\n{\r\n}\r\n```\r\n\r\nAddresses:
https://github.com/elastic/sdh-kibana/issues/3389\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"1892bf65352aa819cefb0408c507ad894edb8a16"}}]}]
BACKPORT-->

Co-authored-by: Matthew Kime <[email protected]>
nkhristinin pushed a commit that referenced this pull request Mar 22, 2023
## Summary

Telemetry no longer makes field caps calls. Also batch loads data view
saved objects.

To collect telemetry - 
```
POST kbn:/api/telemetry/v2/clusters/_stats
{
}
```

Addresses: elastic/sdh-kibana#3389

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Data Views Data Views code and UI - index patterns before 8.0 performance release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) v8.7.0 v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants