Add DevWorkspace-specific metrics #500
Conversation
| @@ -120,6 +121,7 @@ func (r *DevWorkspaceReconciler) Reconcile(req ctrl.Request) (reconcileResult ct | |||
| workspace.Status.Phase = dw.DevWorkspaceStatusStarting | |||
| workspace.Status.Message = "Initializing DevWorkspace" | |||
| err = r.Status().Update(ctx, workspace) | |||
| metrics.WorkspaceStarted(workspace, reqLogger) | |||
There was a problem hiding this comment.
Should not we check error before incrementing Started?
There was a problem hiding this comment.
Won't it increment Started when creating devworkspace with Started: false.
I'm afraid I don't understand why we even set Starting phase here, I would say we just should update status with DevWorkspace, and STOPPED.
There was a problem hiding this comment.
You're right, I don't know what I was thinking doing it this way. I'll fix this before merging, but I got side-tracked with debugging #527 today.
There was a problem hiding this comment.
Should be fixed, though there's still a bit of strangeness in having to set the Started condition ASAP and then also having to do
reconcileStatus := currentStatus{phase: dw.DevWorkspaceStatusStarting}
reconcileStatus.setConditionTrue(conditions.Started, "DevWorkspace is starting")
to ensure the condition isn't wiped out later.
controllers/workspace/status.go
Outdated
| @@ -62,6 +63,7 @@ var healthHttpClient = &http.Client{ | |||
| // Parameters for result and error are returned unmodified, unless error is nil and another error is encountered while | |||
| // updating the status. | |||
| func (r *DevWorkspaceReconciler) updateWorkspaceStatus(workspace *dw.DevWorkspace, logger logr.Logger, status *currentStatus, reconcileResult reconcile.Result, reconcileError error) (reconcile.Result, error) { | |||
| updateMetricsForPhase(workspace, status.phase, logger) | |||
There was a problem hiding this comment.
I assume metrics will be more precise if we move after check that status update error is nil
There was a problem hiding this comment.
Move to only update metrics if the corresponding status update succeeds.
|
|
||
| func incrementStartTimeBucketForWorkspace(wksp *dw.DevWorkspace, log logr.Logger) { | ||
| sourceLabel := wksp.Labels[workspaceSourceLabel] | ||
| hist, err := workspaceStartupTimesHist.GetMetricWith(map[string]string{metricSourceLabel: sourceLabel}) |
There was a problem hiding this comment.
So, we would need to create PR against OpenShift Console, and probably backport it to OS 4.8, so also update existing CR is Console if label is not detected.
There was a problem hiding this comment.
Done (finally): openshift/console#9752
Signed-off-by: Angel Misevski <[email protected]>
Add metrics to count * Workspaces started * Workspaces that successfully entered the Running phase * Workspaces that failed to start Metrics are labelled according to the value of the 'controller.devfile.io/devworkspace-source' label on the DevWorkspace, allowing metrics to be grouped based on the tool creating DevWorkspaces (e.g. Web Terminal Operator, Eclipse Che, etc.) Signed-off-by: Angel Misevski <[email protected]>
Signed-off-by: Angel Misevski <[email protected]>
* Add 'WorkspaceStarted' condition to mark when a workspace enters the 'Starting' phase * Move custom condition definitions to a separate package to avoid circular dependencies when they're used in another package. Signed-off-by: Angel Misevski <[email protected]>
Signed-off-by: Angel Misevski <[email protected]>
* Move setting "Started" condition to the point in the reconcile loop where we know the workspace is started to avoid marking new stopped workspaces as started * Set "Started" condition to false when a workspace is stopped * Only update metrics if the API call to sync workspace status succeeds to avoid accidentally incrementing metrics when a request fails Signed-off-by: Angel Misevski <[email protected]>
amisevsk
force-pushed
the
devworkspace-metrics
branch
from
d943c7b to
33ccf79
Compare
|
/test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amisevsk, JPinkney, sleshchenko 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 |
What does this PR do?
Adds DevWorkspace-specific metrics to the metrics server, tracking
Currently, the metrics come with a
sourcelabel. The value of this label is derived from the value of thecontroller.devfile.io/devworkspace-sourcelabel on the DevWorkspace (optional). If set, this allows metrics to be partitioned based on the tool creating the DevWorkspace (e.g. Web Terminal Operator, Eclipse Che, etc.)Metrics are updated on phase changes in the DevWorkspace (i.e. going from phase 'Started' -> 'Running') to avoid double counting DevWorkspaces.
What issues does this PR fix or reference?
Additional nice-to-haves for #241
Is it tested? How?
Changes can be tested in same way as #405 (note metrics won't appear until they're updated at least once)
A simpler way to test these changes directly is by running the controller locally:
make runthe controller in one windowkubectl apply -f samples/theia-next.yamlPR Checklist
/test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-pathto trigger)v7-devworkspaces-operator-e2e: DevWorkspace e2e testv7-devworkspace-happy-path: DevWorkspace e2e test