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

feat(supervisor): add resource status severity #12157

Merged
merged 1 commit into from
Aug 17, 2022

Conversation

andreafalzetti
Copy link
Contributor

@andreafalzetti andreafalzetti commented Aug 16, 2022

Description

  • Extends status.proto with ResourceStatus severity to remove the duplicated severity calculation from each client
  • Refactor gp-cli and jetbrains backend-plugin to consume the new severity property

Related Issue(s)

Fixes #12075

How to test

  1. Launch a workspace https://afalz-1207846d967e0b.preview.gitpod-dev.com/#https://github.com/gitpod-io/gitpod
  2. Select IntelliJ latest as preferred IDE
  3. Stress the CPU/Memory and using both gp top and JB backend control center, see the cpu/memory thresholds change color to WARN/DANGER

Release Notes

NONE

Documentation

Werft options:

  • /werft with-preview

@andreafalzetti andreafalzetti force-pushed the afalz/12075-severity-thresholds branch 4 times, most recently from eb3399b to 802631f Compare August 16, 2022 16:30
@andreafalzetti andreafalzetti marked this pull request as ready for review August 16, 2022 16:34
@andreafalzetti andreafalzetti requested a review from a team August 16, 2022 16:34
@gitpod-io gitpod-io deleted a comment from werft-gitpod-dev-com bot Aug 16, 2022
Copy link
Member

@jeanp413 jeanp413 left a comment

Choose a reason for hiding this comment

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

To be honest I don't see the benefit of this change, APIs should provide the basic info/data and users of the API can use/interpret it freely. Even though it seems repetitive to map from values and threshold it's fine and the code is really simple.
I also see you are doing a mapping either way in GitpodMetricProvider.kt it's the same in the end. 🤔

@andreafalzetti
Copy link
Contributor Author

andreafalzetti commented Aug 16, 2022

To be honest I don't see the benefit of this change

@jeanp413 Fair point 🤔 From my point of view, we already had inconsistency between gp-cli and JB IDEs having different thresholds [1], from a client perspective, it makes sense to me having only to map severity to color instead of value to color.

@jeanp413
Copy link
Member

it makes sense to me having only to map severity to color instead of value to color

yeah I see that but that's more UI related so the API shouldn't be responsible of that.

Anyway if everybody else aproves just ignore my review 😄

@andreafalzetti
Copy link
Contributor Author

andreafalzetti commented Aug 16, 2022

Anyway if everybody else aproves just ignore my review 😄

It's important that everyone feels that we ship meaningful stuff :) This is a suggestion coming from @akosyakov, I found it valuable and we agred to work on it this week but it's important to challenge ideas so let's have a discussion with the other team members and let's take a decision.

I look forward to get feedback from @felladrin, @iQQBot, @mustard-mh, @loujaybee, @akosyakov :)

➡️ I don't mind closing this PR if we, as a team, decide that we want to keep the threshold values in each client instead of a single place (supervisor).

@akosyakov
Copy link
Member

The main motivation is to having one place to control thresholds, not updating each client if we figure out that danger should .9 not .95.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

it works as advertized, but we don't compute it in supervisor in all branches, see https://github.com/gitpod-io/gitpod/pull/12157/files#r947179792

/hold

@andreafalzetti
Copy link
Contributor Author

/unhold

@roboquat roboquat merged commit 131aa35 into main Aug 17, 2022
@roboquat roboquat deleted the afalz/12075-severity-thresholds branch August 17, 2022 11:35
@roboquat roboquat added deployed: IDE IDE change is running in production deployed Change is completely running in production labels Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: supervisor deployed: IDE IDE change is running in production deployed Change is completely running in production release-note-none size/XXL team: IDE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cpu/memory thresholds should be propagated by supervisor
4 participants