-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Usage Collection] Use PIT for collecting UI/Usage Counters & AppUsage #135689
[Usage Collection] Use PIT for collecting UI/Usage Counters & AppUsage #135689
Conversation
Pinging @elastic/kibana-core (Team:Core) |
@elasticmachine merge upstream |
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.
LGTM, just one question
soRepository: ISavedObjectsRepository, | ||
findOptions: SavedObjectsCreatePointInTimeFinderOptions | ||
): Promise<Array<SavedObjectsFindResult<T>>> { | ||
const finder = soRepository.createPointInTimeFinder<T>({ ...findOptions, perPage: 100 }); |
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.
Overall using a PIT search is definitely an improvement.
I still wonder if 100
is the correct batch size? I would have gone with 1k, to avoid having that much additional roundtrips with ES in case of a lot of SOs
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.
I used 100
only because that's the value we placed in the docs (so I assumed the recommendation). Happy to change it to any other value.
@lukeelmers as the writer of those docs. What do you think?
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.
I can see in the code we are defaulting to 1000
. I'll change it to 1000 then 😇
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.
@lukeelmers as the writer of those docs. What do you think?
Yeah I think I probably just forgot to update the docs when we decided to go with 1000
as the default. There was nothing special about the number 100
.
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
Summary
Resolves #96715
Related to #93770
For maintainers