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

[ws-man-bridge] Add started and completed metrics to track health #9422

Merged
merged 1 commit into from
Apr 22, 2022

Conversation

easyCZ
Copy link
Member

@easyCZ easyCZ commented Apr 20, 2022

Description

Introduce metrics for when an update is received, and when an update is finished processing. The combination of these gives us something like and measurements for the system. This allows us to construct graphs where we can clearly see the following:

  • How much data flows in
  • How much data is completed
  • How much of it errored
  • How long it took to process

It's important to note these are seperate metrics, and should be. Sometimes a system doesn't output anything (due to error) and only reporting metrics on output would prevent us from capturing this case.

This PR also introduces a sub-method which does most of the existing logic, the original method is used to wrap the private one with metircs.

Related Issue(s)

How to test

  • Workspace starts work
  • Prebuilds work

Release Notes

NONE

Documentation

NONE

/hold

@easyCZ easyCZ force-pushed the mp/wsman-update-metrics branch from 3110629 to b0d0db3 Compare April 21, 2022 07:27
@roboquat roboquat added size/L and removed size/XL labels Apr 21, 2022
@easyCZ easyCZ force-pushed the mp/wsman-update-metrics branch 4 times, most recently from 109d197 to b3e0584 Compare April 21, 2022 12:38
@easyCZ easyCZ marked this pull request as ready for review April 21, 2022 13:30
@easyCZ easyCZ requested a review from a team April 21, 2022 13:30
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Apr 21, 2022
@easyCZ easyCZ changed the title [ws-man] Add started and completed metrics to track health [ws-man-bridge] Add started and completed metrics to track health Apr 21, 2022
@easyCZ
Copy link
Member Author

easyCZ commented Apr 21, 2022

/werft run

👍 started the job as gitpod-build-mp-wsman-update-metrics.7

@geropl geropl self-assigned this Apr 22, 2022
@geropl
Copy link
Member

geropl commented Apr 22, 2022

BTW: If a PR seems otherwise ready, but still has a "hold" label, I tend to be slightly put off to just start reviewing it.
Is there a reason this PR still has it? Would it make sense to add sth akin "make sure to remove the label before marking a PR as "review ready"" to our "how we work"? 🤔

@easyCZ
Copy link
Member Author

easyCZ commented Apr 22, 2022

Is there a reason this PR still has it? Would it make sense to add sth akin "make sure to remove the label before marking a PR as "review ready"" to our "how we work"? 🤔

I want to land them myself, so that I'm in control of the order of PRs going in and can ensure I know what has landed and what hasn't. It's a personal workflow. I tried to change it such that hold is on PRs by default but didn't get consensus as it seems it'd interfere with workflows of others.

IMO, we'll need to move this down the line in particular with stacked PRs as it's unlikely others will have the right context to know when exactly changes should land. In particular with feature flags, there may be some additional validation the author may want to do ahead of landing the change (ensure prod is disabled in the feature, for example). This in general goes back to putting more responsiblity on the author of the PR, and less on the reviewer - trust by default.

@easyCZ
Copy link
Member Author

easyCZ commented Apr 22, 2022

/unhold

@easyCZ
Copy link
Member Author

easyCZ commented Apr 22, 2022

/hold

@geropl
Copy link
Member

geropl commented Apr 22, 2022

Got this while testing:
💯 unrelated, but blocks testing.
image

@easyCZ
Copy link
Member Author

easyCZ commented Apr 22, 2022

Thanks @geropl, will investigate. Sorry about that, preview looks to be broken more often than not..

@easyCZ easyCZ force-pushed the mp/wsman-update-metrics branch from b3e0584 to 9074f67 Compare April 22, 2022 08:08
@geropl
Copy link
Member

geropl commented Apr 22, 2022

/werft run with-vm=true

👍 started the job as gitpod-build-mp-wsman-update-metrics.9

@easyCZ Got the same error, and just made very good experience with with-vm=true, so took the liberty to re-trigger 👍

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.

After minutes of image pull I was able to use my workspace 😌

@easyCZ
Copy link
Member Author

easyCZ commented Apr 22, 2022

Thanks @geropl!

@easyCZ
Copy link
Member Author

easyCZ commented Apr 22, 2022

/unhold

@roboquat roboquat merged commit 5357c9a into main Apr 22, 2022
@roboquat roboquat deleted the mp/wsman-update-metrics branch April 22, 2022 11:32
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/M team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants