Skip to content

Conversation

@mustard-mh
Copy link
Contributor

@mustard-mh mustard-mh commented Oct 24, 2024

Description

Why we implement heartbeat implement on supervisor? Because it's hard to change token scope in JB Gateway (tech-debt) properly. Use supervisor to implement it is the easiest way to do so: we don't care when request failed, just need users know that this feature requires a newer installation

Related Issue(s)

Fixes CLC-803

How to test

Integration tests
Integration tests of current PR should pass

Smoke test it doesn't break JB Gateway

Test heartbeat

  • Use catfood.gitpod.cloud as your Gitpod Host (in JB preferences > Tools > Gitpod > Host). You will see errors when send heartbeat as supervisor is not deployed yet
  • Use current preview env as your Gitpod Host, there's no such error

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=jetbrains
    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/L labels Oct 31, 2024
@mustard-mh mustard-mh changed the title WIP: test additional hb [JBGateway] connection-based client side additional heartbeat Oct 31, 2024
@mustard-mh mustard-mh marked this pull request as ready for review October 31, 2024 17:12
@mustard-mh mustard-mh requested review from a team as code owners October 31, 2024 17:12
connectionLifetime.launch {
while (isActive) {
val delaySeconds = 30 + nextInt(5, 15)
if (thinClientJob?.isActive == true) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this where the actual additional heartbeating is done? The way I read it seems to me like it's only once, and I'm not really sure what the underlying signal is we use to send it.

Copy link
Contributor Author

@mustard-mh mustard-mh Nov 5, 2024

Choose a reason for hiding this comment

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

We will send it when isActive is true and thinClientJob?.isActive is true:

  • isActive means the context connectionLifetime is active, the there's a connection exists
  • thinClientJob?.isActive means there's a IDE client shows on user's desktop

So it can be: connectionLifetime is ready, but thin client is downloading (? I don't verify it).

repeated string envs = 1;
}

message SendHeartBeatRequest {}
Copy link
Member

Choose a reason for hiding this comment

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

Would it be useful to include something like a reason here for debugging purposes?

Comment on lines +196 to +197
// Example for macOS ~/Library/Application Support/JetBrains/JetBrainsGateway2024.3/plugins
//
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this comment was confusing to me at first. I think if we get rid of the part after macOS here, it will make more sense (it seemed at first like you would provide the contents of some plugin file).

@filiptronicek
Copy link
Member

@mustard-mh code looks good, but I am still not certain what it actually does. Would you be available to sync?

Copy link
Member

@filiptronicek filiptronicek left a comment

Choose a reason for hiding this comment

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

Looks good to me. I have not actually tried testing if this properly postpones timeouts, but have reviewed the code and it at least does not seem to break anything 😄.

Thank you a bunch for picking this up!

(also, left one small copy comment that I think is worth tackling. It also looks like the integration tests are somehow broken?)

row {
checkBox("Persistent connection heartbeats")
.bindSelected(state::additionalHeartbeat)
.comment("Keep workspaces running as long as the IDE connection remains active.")
Copy link
Member

Choose a reason for hiding this comment

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

nit: we don't end with a period in the checkbox above, and neither do we in the Dashboard.

Suggested change
.comment("Keep workspaces running as long as the IDE connection remains active.")
.comment("Keep workspaces running as long as the IDE connection remains active")

@roboquat roboquat merged commit b6c243b into main Nov 5, 2024
47 of 48 checks passed
@roboquat roboquat deleted the hw/gw-hb branch November 5, 2024 16:50
mustard-mh added a commit that referenced this pull request Nov 6, 2024
roboquat added a commit that referenced this pull request Nov 7, 2024
…P) (#20320)

* Update Platform Version of JetBrains Gateway Plugin (EAP) to

* Revert gradle properties changes

* Address Filip's feedback

#20319 (comment)

Co-authored-by: Filip Troníček <[email protected]>

---------

Co-authored-by: Huiwen <[email protected]>
Co-authored-by: Filip Troníček <[email protected]>
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.

4 participants