Skip to content

[server] Add counter metric for image build starts #14204

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
Merged
merged 2 commits into from
Nov 1, 2022
Merged

[server] Add counter metric for image build starts #14204

merged 2 commits into from
Nov 1, 2022

Conversation

andrew-farries
Copy link
Contributor

@andrew-farries andrew-farries commented Oct 26, 2022 •
edited
Loading

Description

Re-do of #14185 (reverted in #14203), this time incrementing the metric only on image builds.

Related Issue(s)

Part of #12960

How to test

  • Port-forward to the server metrics port: kubectl port-forward deploy/server 9500:9500
  • Hit the server metrics endpoint at :9500/metrics and look for the gitpod_server_image_builds_started_total metric.
  • Start an image build
  • See the counter increase.
  • Start a workspace that doesn't require an image build.
  • See that the counter does not increase.

Release Notes

NONE

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

@andrew-farries andrew-farries requested a review from a team October 26, 2022 15:55
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-af-fix-server-image-build-total-metric.6 because the annotations in the pull request description changed
(with .werft/ from main)

@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Oct 26, 2022
@andrew-farries andrew-farries mentioned this pull request Oct 26, 2022
Merged
4 tasks
@andrew-farries
Copy link
Contributor Author

/hold

we should deploy the revert (#14203) first to clear the faulty metric from server prometheus registry.

Base automatically changed from af/revert-14185 to main October 26, 2022 18:40
@roboquat roboquat added size/XS and removed size/S labels Oct 26, 2022
easyCZ
easyCZ reviewed Oct 26, 2022
View reviewed changes
components/server/src/workspace/workspace-starter.ts
@@ -1257,6 +1256,10 @@ export class WorkspaceStarter {

const result = await client.build({ span }, req, imageBuildLogInfo);

if (result.actuallyNeedsBuild) {
increaseImageBuildsStartedTotal();
Copy link
Member

Choose a reason for hiding this comment

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

At what point does this increment? Is it after we've completed the build? Or is the build method async and it tells us it needs to be build and returns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the build call on line 1257 awaits until the build starts:

gitpod/components/image-builder-api/typescript/src/sugar.ts

Lines 176 to 177 in c551c2c

// build returns a nested promise. The outer one resolves/rejects with the build start,
// the inner one resolves/rejects when the build is done.

so we're incrementing on build start here, not completion.

The build completion is awaited further down this file on line 1281:

gitpod/components/server/src/workspace/workspace-starter.ts

Line 1281 in c551c2c

buildResult = await result.buildPromise;

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@andrew-farries andrew-farries force-pushed the af/fix-server-image-build-total-metric branch from 0dd057c to c551c2c Compare October 31, 2022 13:54
@roboquat roboquat added size/S and removed size/XS labels Oct 31, 2022
@andrew-farries andrew-farries mentioned this pull request Oct 31, 2022
Merged
4 tasks
easyCZ
easyCZ approved these changes Oct 31, 2022
View reviewed changes
components/server/src/workspace/workspace-starter.ts
@@ -1257,6 +1256,10 @@ export class WorkspaceStarter {

const result = await client.build({ span }, req, imageBuildLogInfo);

if (result.actuallyNeedsBuild) {
increaseImageBuildsStartedTotal();
Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-af-fix-server-image-build-total-metric.9 because the annotations in the pull request description changed
(with .werft/ from main)

@roboquat roboquat merged commit edaada8 into main Nov 1, 2022
@roboquat roboquat deleted the af/fix-server-image-build-total-metric branch November 1, 2022 08:56
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@easyCZ easyCZ easyCZ approved these changes

Assignees
No one assigned
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.
3 participants
@andrew-farries @easyCZ @roboquat