Skip to content

Conversation

kylos101
Copy link
Contributor

@kylos101 kylos101 commented Sep 10, 2024

Description

Introduce RED metrics for ws-proxy for client and server requests. This also lets us inspect http_version (as a label) on requests.

@akosyakov originally started this work in #17294 (it never landed on main). @kylos101 fixed a couple minor things (see later commits in this PR) and tested.

Related Issue(s)

Fixes ENT-673

How to test

Functional test with HTTP2 ✅

  1. Setup a port forward for grafana in the preview environment for this PR, kubectl port-forward deployment/grafana -n monitoring-satellite 3000 (add observability if it is missing with leeway run dev/preview:deploy-monitoring-satellite).
  2. Start a workspace in the preview environment from our demo-docker repo (it starts a voting and runs them on ports)
  3. Open the voting app), it'll manifests as HTTP/1.1 traffic (you can see this in the logs from the workspace running in preview)
  4. Inspect Grafana data (queries below)

sum by(resource, http_version) (rate(gitpod_ws_proxy_http_server_requests_total[5m]))
sum by(resource, http_version) (rate(gitpod_ws_proxy_http_client_requests_total[5m]))

Results here.

Functional test with HTTP2 disabled in Chrome 🚫

  1. Close all Chrome instances
  2. Start Chrome using the --disable-http2 flag
  3. Repeat the test steps (above), the client metrics should be different, but they are not ‼️ We force attempt HTTP2 here.
    • Perhaps this is different when customer networks use ZScaler, and downgrade traffic to HTTP/1.1? Either way, disabling HTTP2 in chrome doesn't change the behavior.

Loadgen in an ephemeral cluster 🚫

The ops repo does not allow for ephemeral clusters to be created from installer images that are not from main-gha. So, we first need to land this PR on main, and then test in an ephemeral cluster.

dogfood this change in catfood ⌛

Test on Monday, Sept 16, after it lands on main.

Documentation

Preview status

Gitpod was successfully deployed to your preview environment.

Build Options

Build
  • /werft with-werft
    Run the build with werft instead of GHA
  • leeway-no-cache
  • /werft no-test
    Run Leeway with --dont-test
Publish
  • /werft publish-to-npm
  • /werft publish-to-jb-marketplace
Installer
  • analytics=segment
  • with-dedicated-emulation
  • workspace-feature-flags
    Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • /werft preemptible
    Saves cost. Untick this only if you're really sure you need a non-preemtible machine.
  • with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh. If enabled, with-preview and with-large-vm will be enabled.
  • with-monitoring

/hold

@roboquat roboquat added size/XL and removed size/S labels Sep 11, 2024
@kylos101
Copy link
Contributor Author

kylos101 commented Sep 12, 2024

My browser is connecting to proxy with http/2, but when proxy does the reverse proxy to server, it downgrades to 1.1.

http/2 from my browser:
image

http/1.1 as seen by server:
image

server is using Express, but we haven't changed the impl to use http2 (which would be required for it to leverage http2, I think). Ref: https://stackoverflow.com/a/59569226

kylos101 and others added 3 commits September 12, 2024 17:58
Originally from #17294

Co-authored-by: Anton Kosyakov <[email protected]>
I think for this value to be populated, we'll need to "bubble up" httpVersion (like what was done with many methods and resource) 🤔 Think of a better way.
@kylos101 kylos101 changed the title Kylos101/ent 673 [ws-proxy] introduce RED metrics, including a http_version label Sep 12, 2024
@kylos101
Copy link
Contributor Author

Testing server requests:
image

Testing client requests:
image

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Code changes LGTM, with this spot where I think we can improve readability. ✔️

Did not test due to time, though.

@roboquat roboquat merged commit 2be52da into main Sep 16, 2024
15 of 16 checks passed
@roboquat roboquat deleted the kylos101/ENT-673 branch September 16, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants