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.

Sign up for GitHub

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

Jump to bottom

[prebuilds] increment metric only with state change #10621

Merged
merged 1 commit into from
Jun 13, 2022
Merged

[prebuilds] increment metric only with state change #10621

merged 1 commit into from
Jun 13, 2022

Conversation

laushinka
Copy link
Contributor

Description

This PR makes sure that metrics are only incremented when a prebuild state changes.
It also extends the logging information with the logging context.

Related Issue(s)

Fixes #10616

How to test

  1. Trigger multiple prebuilds (does not have to be at the same time) and wait until they are completed.
  2. Check that the metrics show up on this page for started and completed.

Release Notes

NONE

Documentation

@laushinka laushinka requested a review from a team June 13, 2022 10:07
@laushinka laushinka requested a review from geropl June 13, 2022 10:07
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Jun 13, 2022
@andrew-farries andrew-farries self-assigned this Jun 13, 2022
andrew-farries
andrew-farries reviewed Jun 13, 2022
View reviewed changes
Copy link
Contributor

@andrew-farries andrew-farries left a comment

Choose a reason for hiding this comment

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

The links in the 'How to test' section don't work for me. Here's what I did:

  1. Set my kube context to your preview environment.
  2. Port forwarded to the prometheus-k8s service.
  3. Made an SSH tunnel from my local machine to my workspace so I could access prometheus at localhost.
  4. Started a few prebuilds and watched the gitpod_prebuilds_started_total and gitpod_prebuilds_completed_total metrics in the prometheus UI.

Things look basically ok to me; for every prebuild I started I saw the started counter increase, and when it completed I saw completed increase.

The counter for the prebuild start increased when the imagebuild was started though, not exactly when the prebuild started. Not sure if this is desired behaviour.

@laushinka
Copy link
Contributor Author

The links in the 'How to test' section don't work for me.

@andrew-farries Sorry this was probably because my workspace timed out. Thanks for managing to test despite it!

The counter for the prebuild start increased when the imagebuild was started though, not exactly when the prebuild started. Not sure if this is desired behaviour.

The increment should happen once we've stored a prebuilt workspace with a queued state.

@laushinka laushinka force-pushed the laushinka/fix-prebuild-state-metrics-10616 branch from f5429ef to c242593 Compare June 13, 2022 14:17
@laushinka laushinka force-pushed the laushinka/fix-prebuild-state-metrics-10616 branch from c242593 to ff6d055 Compare June 13, 2022 14:19
@laushinka laushinka requested a review from geropl June 13, 2022 14:21
geropl
geropl reviewed Jun 13, 2022
View reviewed changes
components/ws-manager-bridge/ee/src/prebuild-updater-db.ts
@@ -80,7 +80,15 @@ export class PrebuildUpdaterDB implements PrebuildUpdater {
span.setTag("updatePrebuildWorkspace.prebuild.state", updatedPrebuild.state);
span.setTag("updatePrebuildWorkspace.prebuild.error", updatedPrebuild.error);

if (writeToDB && updatedPrebuild.state && terminatingStates.includes(updatedPrebuild.state)) {
// Here we make sure that we increment the counter only when:
Copy link
Member

Choose a reason for hiding this comment

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

❤️

geropl
geropl approved these changes Jun 13, 2022
View reviewed changes
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.

Awesome, code LGTM, thx for the comments and logCtx fixex! 🙏

@roboquat roboquat merged commit 025c275 into main Jun 13, 2022
@roboquat roboquat deleted the laushinka/fix-prebuild-state-metrics-10616 branch June 13, 2022 14:38
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrew-farries andrew-farries andrew-farries left review comments

@geropl geropl geropl approved these changes

Assignees

@andrew-farries andrew-farries

Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/S team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix prebuild state metrics
4 participants
@laushinka @andrew-farries @geropl @roboquat