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

Implement Jetbrains IDEs heartbeating #6152

Merged
merged 1 commit into from
Oct 14, 2021
Merged

Conversation

atduarte
Copy link
Contributor

Description

Create the IntelliJ backend plugin to provide support for Gitpod.

When installed in the headless IntelliJ running in a Gitpod workspace, this plugin monitors user activity of the client IntelliJ and sends heartbeats accordingly. Avoiding the workspace timing out.

Related Issue(s)

Addresses #5643

How to test

Manually tests are required.

  1. Set up a workspace with the headless Jetbrains IDE.
  2. Produce the plugin by running ./gradlew buildPlugin.
  3. Unzip build/distributions/gitpod-intellij-plugin-1.0-SNAPSHOT.zip to the plugins/ folder of the headless Jetbrains IDE.
  4. Start the headless Jetbrains IDE.

Release Notes

NONE

Documentation

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@atduarte atduarte requested review from corneliusludmann and akosyakov and removed request for laushinka October 11, 2021 14:21
@corneliusludmann
Copy link
Contributor

Hi @atduarte! I just saw that the build fails because there are missing license headers. You can fix this by running

$ leeway run components:update-license-header

in a Gitpod workspace and committing the changes.

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@corneliusludmann
Copy link
Contributor

corneliusludmann commented Oct 11, 2021

Just thinking out loud: What do you all think of moving this plugin closer to the IntelliJ IDE image. E.g. something like

components/ide/intellij/plugin/

for this Java plugin and the IDE images go to:

components/ide/intellij/pycharm/
components/ide/intellij/webstorm/
components/ide/intellij/idea/
...

(still struggling with the different JetBrains terms—don't know if intellij is the proper name for the main folder or jetbrains or whatever)

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@atduarte
Copy link
Contributor Author

@corneliusludmann had to change the approach because of the way werft works. With this new approach, we publish the libs to the local maven repository and the plugin will fetch from there. Wdyt?

Also moved to components/ide/jetbrains/backend-plugin

@github-actions
Copy link
Contributor

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yaml was changed and it might be harmful.

@codecov
Copy link

codecov bot commented Oct 12, 2021

Codecov Report

Merging #6152 (cb09a69) into main (9d4bfb5) will increase coverage by 14.68%.
The diff coverage is n/a.

❗ Current head cb09a69 differs from pull request most recent head 7130360. Consider uploading reports for the commit 7130360 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##             main    #6152       +/-   ##
===========================================
+ Coverage   19.04%   33.72%   +14.68%     
===========================================
  Files           2      135      +133     
  Lines         168    22708    +22540     
===========================================
+ Hits           32     7659     +7627     
- Misses        134    14366    +14232     
- Partials        2      683      +681     
Flag Coverage Δ
components-blobserve-app 28.38% <ø> (?)
components-common-go-lib 36.89% <ø> (?)
components-content-service-api-go-lib ∅ <ø> (?)
components-content-service-app 14.48% <ø> (?)
components-content-service-lib 14.48% <ø> (?)
components-ee-agent-smith-app 25.52% <ø> (?)
components-ee-kedge-app 45.48% <ø> (?)
components-ee-ws-scheduler-app 63.16% <ø> (?)
components-gitpod-cli-app 9.74% <ø> (?)
components-gitpod-protocol-go-lib ∅ <ø> (?)
components-image-builder-api-go-lib ∅ <ø> (?)
components-image-builder-app 34.44% <ø> (?)
components-image-builder-bob-app ∅ <ø> (?)
components-image-builder-mk3-app 34.46% <ø> (?)
components-licensor-lib 74.10% <ø> (?)
components-local-app-api-go-lib ∅ <ø> (?)
components-local-app-app-darwin-amd64 ∅ <ø> (?)
components-local-app-app-darwin-arm64 ∅ <ø> (?)
components-local-app-app-linux-amd64 19.04% <ø> (ø)
components-local-app-app-linux-arm64 ∅ <ø> (∅)
components-local-app-app-windows-386 ∅ <ø> (∅)
components-local-app-app-windows-amd64 ∅ <ø> (∅)
components-local-app-app-windows-arm64 ∅ <ø> (∅)
components-openvsx-proxy-app 46.22% <ø> (?)
components-registry-facade-api-go-lib ∅ <ø> (?)
components-registry-facade-app 11.63% <ø> (?)
components-registry-facade-lib 11.63% <ø> (?)
components-service-waiter-app ∅ <ø> (?)
components-supervisor-api-go-lib ∅ <ø> (?)
components-supervisor-app 38.05% <ø> (?)
components-workspacekit-app 7.09% <ø> (?)
components-ws-daemon-api-go-lib ∅ <ø> (?)
components-ws-daemon-app 22.11% <ø> (?)
components-ws-daemon-nsinsider-app ∅ <ø> (?)
components-ws-manager-api-go-lib ∅ <ø> (?)
components-ws-manager-app 39.26% <ø> (?)
components-ws-manager-bridge-api-go-lib ∅ <ø> (?)
components-ws-proxy-app 69.04% <ø> (?)
dev-blowtorch-app ∅ <ø> (?)
dev-loadgen-app ∅ <ø> (?)
dev-poolkeeper-app ∅ <ø> (?)
dev-sweeper-app ∅ <ø> (?)
dev-version-manifest-app 83.56% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/supervisor/pkg/supervisor/supervisor.go 6.25% <0.00%> (ø)
...omponents/ee/agent-smith/pkg/signature/sinature.go 44.27% <0.00%> (ø)
components/ws-manager/pkg/manager/annotations.go 65.11% <0.00%> (ø)
...omponents/registry-facade/pkg/registry/manifest.go 9.65% <0.00%> (ø)
components/ws-manager/pkg/clock/clock.go 68.62% <0.00%> (ø)
components/blobserve/pkg/blobserve/refstore.go 61.80% <0.00%> (ø)
components/gitpod-cli/cmd/gitpodRun.go 4.34% <0.00%> (ø)
components/gitpod-cli/cmd/root.go 0.00% <0.00%> (ø)
components/ws-proxy/pkg/proxy/infoprovider.go 60.06% <0.00%> (ø)
components/supervisor/pkg/ports/ports-config.go 79.85% <0.00%> (ø)
... and 123 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d4bfb5...7130360. Read the comment docs.

@akosyakov
Copy link
Member

@corneliusludmann Could you review it? We need to check usual staff also, i.e. copyrights, leeway builds and so on.

I am not sure how we should put plugins, each plugin as own component or has jetbrains folder and put everything there? We have 2 plugins for now, but not sure how it is going to develop later. cc @csweichel

@corneliusludmann
Copy link
Contributor

Hey @atduarte! I started to review your PR. 🙏

Regarding the CI build:

  1. You added the plugin to :all-apps of components/BUILD.yaml which makes sense. Once we have the Jetbrains IDE image in place, this Docker image will have this plugin as dependencies. At this point having it in :all-apps seems legit. However, during the CI build :all is executed which does not has :all-apps as a dependency (it has only :all-docker). That's why this plugin is not built currently. I would suggest to keep this in :all for now and remove it from :all in the PR that adds the IDE image which is built during :all-docker (same for supervisor and protocol Java API packages).
  2. I moved it to :all in this werft build for testing reasons which show that the approach with the local maven repo does not work, unfortunately. 😞 The reason is that each component build is isolated and therefore they don't share a common local maven repo folder. Even if I think that the local maven repo approach would be really great, I think the easiest path to get this working is to switch back to your approach with the subproject that you had in 12d0a48. I would propose to set the actual subproject path as property similar to this: d8ab37f. What do you think?

@akosyakov
Copy link
Member

I missed #6152 (comment) first time. I liked the idea. I think we should call it components/ide/jetbrains top directory, intellij is just one product.

@atduarte
Copy link
Contributor Author

@corneliusludmann sounds good. Will proceed with the changes. Thank you!

@akosyakov I guess you are associating "IntelliJ" with "IntelliJ IDEA" but that's not entirely correct. The "IntelliJ Platform" is what powers all Jetbrains IDEs [1], and the plugin is using the "IntelliJ Platform Plugin SDK" [2]. That said, I believe the folder being intellij or jetbrains is indifferent and currently is components/ide/jetbrains/backend-plugin.

@corneliusludmann
Copy link
Contributor

Looks great! 🥳

/lgtm

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 31017ca447c057ec3e93de5fe108e004366246ae

@corneliusludmann
Copy link
Contributor

@akosyakov You need to approve this as well because team-ide is not an owner of https://github.com/gitpod-io/gitpod/blob/291bcd9f259ae5e48ab73311e68a4970ea89a432/components/supervisor-api/OWNERS.

(is it on purpose?)

@akosyakov
Copy link
Member

@csweichel Is it fine if we change ownership for supervisor-api and localapp-api to IDE?


object AuthTokenService {
private val logger = logger<AuthTokenService>()
private const val SUPERVISOR_ADDRESS = "localhost:22999"
Copy link
Member

Choose a reason for hiding this comment

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

We should not hard code but fetch or access env vars, but fetch all info from supervisor. All env vars and ports are subject to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that this is the address of the supervisor—needed to establish the connection—, how do you propose to get it without hard coding or access env vars?

Copy link
Member

Choose a reason for hiding this comment

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

ah, right this only env var on which you can rely, the rest should be fetch from the supervisor.

Copy link
Member

Choose a reason for hiding this comment

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

.build()

val request = GetTokenRequest.newBuilder()
.setHost(System.getenv("GITPOD_HOST").split("//").last())
Copy link
Member

Choose a reason for hiding this comment

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

the same here, it should come from the supervisor info endpoint

@Suppress("MagicNumber")
private val intervalInSeconds = 30

private val uri = "wss://${System.getenv("GITPOD_HOST").split("//").last()}/api/v1"
Copy link
Member

Choose a reason for hiding this comment

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

we should use the superviso info ednpoint to fetch all these data

@akosyakov
Copy link
Member

I don't see the plugin using supervisor api to get configuration. Is it going to be addressed with the next PR? We should not read any Gitpod info from env vars or hardcode it.

@akosyakov
Copy link
Member

akosyakov commented Oct 14, 2021

Does kotlin language support work for you? I cannot use find references and navigation?

} catch (e: Exception) {
// If connection fails for some reason,
// remove the reference to the existing server.
server.set(null)
Copy link
Member

Choose a reason for hiding this comment

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

it looks strange, usually reconneciton should be handled on transport level, client code should not care, create one instance and use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, that's not the case. We could wrap the server and handle that separately, but I don't see a great benefit. Do you have any specific concerns?

Copy link
Member

Choose a reason for hiding this comment

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

I need to check what we do under the hood, concern is that we leak underlying transport (web sockets) if the error is not about closing, but some application level error, like invalid json and so on.

@akosyakov
Copy link
Member

/lgtm

I looked through the code and left some comments. I think we can merge but they should be addressed with next PRs. I am also not sure about using Kotlin if the language support in VS Code does not work.

/hold

please remove hold if you want to merge it immediately

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akosyakov, corneliusludmann

Associated issue: #5643

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@atduarte
Copy link
Contributor Author

atduarte commented Oct 14, 2021

I don't see the plugin using supervisor api to get configuration. Is it going to be addressed with the next PR? We should not read any Gitpod info from env vars or hardcode it.

What do you propose in regards to getting "CWM_HOST_STATUS_OVER_HTTP_TOKEN"?

Does kotlin language support work for you? I cannot use find references and navigation?

I was surprised by the same thing. The good news is that you will be able to use IntelliJ IDEA soon 😅 I can switch the language to Java in a subsequent PR

@akosyakov
Copy link
Member

akosyakov commented Oct 14, 2021

What do you propose in regards to getting "CWM_HOST_STATUS_OVER_HTTP_TOKEN"?

Is it configured by Intellij IDE then it is fine. It is about Gitpod env vars. We should use services to access them, env vars are implementation details.

I was surprised by the same thing. The good news is that you will be able to use IntelliJ IDEA soon 😅 I can switch the language to Java in a subsequent PR

Yes, we need a setup then to dogfood from IntelliJ rather soon. We don't need to switch to Java in this case, I prefer Kotlin as well. it is just we should have good DX for us as well.

@atduarte
Copy link
Contributor Author

@akosyakov @corneliusludmann I suggest we merge as it is, making the changes in the next PR, since from what I understood the requests are not a blocker and I'll only be back next week (I'm working part-time on Monday, Wednesday, and Thursday mornings).

@akosyakov
Copy link
Member

/unhold

@roboquat roboquat merged commit acee903 into main Oct 14, 2021
@roboquat roboquat deleted the ad/intellij-backend-plugin branch October 14, 2021 12:43
@akosyakov
Copy link
Member

ok, I added the follow up task to our epic

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.

None yet

5 participants