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

[Sessions] Add meta.size to bfetched responses #95774

Closed
wants to merge 1 commit into from

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Mar 30, 2021

Summary

Since we want to avoid stringifying responses to calculate their size, after consulting with @ppisljar, I added a new meta section to the response coming from any bfetch function.
It will contain the response's size in bytes, so it can then be used anywhere it's needed.
Specifically, I intend to use it in #92439 for its cache eviction logic.

I split this out to a separate PR to simplify testing and reviewing.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@lizozom lizozom requested a review from a team as a code owner March 30, 2021 11:29
@botelastic botelastic bot added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label Mar 30, 2021
@lizozom lizozom self-assigned this Mar 30, 2021
@lizozom lizozom added auto-backport Deprecated - use backport:version if exact versions are needed Feature:Search Querying infrastructure in Kibana Team:AppServices v7.13.0 v8.0.0 labels Mar 30, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@lizozom lizozom added the release_note:skip Skip the PR/issue when compiling release notes label Mar 30, 2021
@@ -134,6 +135,11 @@ export const createStreamingBatchedFunction = <Payload, Result extends object>(
if (response.error) {
items[response.id].future.reject(response.error);
} else if (response.result !== undefined) {
if (typeof response.result === 'object') {
response.result.meta = {
Copy link
Member

Choose a reason for hiding this comment

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

i don't really like it that we are adding things to actual response, i would preffer to se meta information separate from actual responses.

also, i think this information could be useful on the server as well, and i would really prefer to keep our server/client API as similar as possible.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
bfetch 11.1KB 11.2KB +78.0B

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

cc @lizozom

Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

I'm worried that the responses now may contain this meta property which is not expected. And it is unfortunate to add the BatchResultBase interface, as Peter also mentioned.

Could you please make it such that the result data is not modified, i.e. with the extra meta property? One way that comes to mind could be to return the whole response, like so (instead of just response.result):

image

That way you can use response.json.length to get the size of the raw response. And response.result to get the response data as before.

@lizozom
Copy link
Contributor Author

lizozom commented Mar 30, 2021

@streamich yep, thanks!
I agree with you both and that's going to be my current approach 🙏🏻

@lizozom
Copy link
Contributor Author

lizozom commented Jun 6, 2021

@elasticmachine merge upstream

@lizozom lizozom closed this Jun 6, 2021
@kibanamachine
Copy link
Contributor

ignoring request to update branch, pull request is closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants