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

Disable bfetch in serverless #183096

Merged
merged 9 commits into from
May 30, 2024
Merged

Conversation

lukasolson
Copy link
Member

@lukasolson lukasolson commented May 9, 2024

Summary

Part of #181938.

Disables bfetch in serverless by overriding the bfetch:disable advanced setting in the serverless.yml.

Bfetch was introduced to bypass the browser connection limit when multiple search requests are made. Here's a picture of what the network tab looks like with bfetch turned off in http1:

image

As the number of requests reaches the connection limit (6 on most modern browsers), requests stall until a connection is available.

In Cloud/serverless, we have a http2 proxy. Http2 does not have this browser connection limit. Turning off bfetch allows us to collect metrics to decide if we want to remove it altogether:

Bfetch enabled Bfetch disabled (this PR)
image image

In my tests, the overall time for a sample dashboard to load was actually smaller with bfetch disabled on http2 vs. bfetch enabled.

Checklist

@lukasolson lukasolson added the Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Data, DataViews) label May 9, 2024
@lukasolson lukasolson self-assigned this May 9, 2024
@lukasolson
Copy link
Member Author

/ci

@lukasolson
Copy link
Member Author

/ci

@lukasolson lukasolson marked this pull request as ready for review May 13, 2024 14:21
@lukasolson lukasolson requested review from a team as code owners May 13, 2024 14:21
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

Code looks good and work well!

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

Just FMI, could you tell me why we're doing it?

@lukasolson
Copy link
Member Author

Just FMI, could you tell me why we're doing it?

I forgot to link to the issue, but there's more context here: #181938

@lukasolson lukasolson added the release_note:skip Skip the PR/issue when compiling release notes label May 14, 2024
@afharo
Copy link
Member

afharo commented May 15, 2024

@lukasolson, have you considered using the labels ci:project-deploy-* to test them on the actual serverless architecture before merging?

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

Can we double check the response is actually gzip encoded, or if it is the raw json? Just glanced at it, and wasn't 100% clear to me.

@lukasolson
Copy link
Member Author

Can we double check the response is actually gzip encoded, or if it is the raw json? Just glanced at it, and wasn't 100% clear to me.

Spun up serverless locally to check, and it looks like it is gzip encoded:

image

@lukasolson lukasolson added ci:project-deploy-elasticsearch Create an Elasticsearch Serverless project ci:project-deploy-observability Create an Observability project ci:project-deploy-security Create a Security Serverless Project labels May 15, 2024
Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Tested and LGTM

@kibana-ci
Copy link
Collaborator

kibana-ci commented May 30, 2024

💚 Build Succeeded

Metrics [docs]

Canvas Sharable Runtime

The Canvas "shareable runtime" is an bundle produced to enable running Canvas workpads outside of Kibana. This bundle is included in third-party webpages that embed canvas and therefor should be as slim as possible.

id before after diff
module count - 5414 +5414
total size - 8.8MB +8.8MB

History

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

cc @lukasolson

@lukasolson lukasolson merged commit 37215fb into elastic:main May 30, 2024
20 checks passed
@kibanamachine kibanamachine added v8.15.0 backport:skip This commit does not require backporting labels May 30, 2024
vitaliidm added a commit that referenced this pull request May 31, 2024
…ts (#184581)

## Summary

- #183096 PR disabled bfetch
requests for Serverless, leading to failure of multiple serverless ES|QL
tests that rely on intercepting `bsearch` request
- addresses:
  -  #184558
  -  #184556
  - #184557
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:project-deploy-elasticsearch Create an Elasticsearch Serverless project ci:project-deploy-observability Create an Observability project ci:project-deploy-security Create a Security Serverless Project release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Data, DataViews) v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants