Skip to content

Add annotation to Web Terminals to enable Operator metrics #9752

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

Merged

Conversation

amisevsk
Copy link
Contributor

@amisevsk amisevsk commented Aug 6, 2021

Description

PR devfile/devworkspace-operator#500 in the DevWorkspace Operator (which supports Web Terminal/CloudShell functionality on the cluster) enables metrics for DevWorkspace custom resources. These metrics are bucketed according to the controller.devfile.io/devworkspace-source annotation, allowing the operator to report metrics such as startup time, failure rate, etc. partitioned based on the tool used to create the CR.

This PR adds the controller.devfile.io/devworkspace-source annotation to web terminals so that DevWorkspace Operator metrics can report Web Terminal-specific metrics.

Tests are not updated as the annotation is optional and does not impact end-user experience. Metrics are still reported without it, but may be less accurate due to other DevWorkspace CRs being included in the data.

Add DevWorkspace Operator supported annotation
'controller.devfile.io/devworkspace-source' to Web Terminal instances to
enable the DevWorkspace Operator to report Web Terminal-specific
metrics.

Signed-off-by: Angel Misevski <[email protected]>
@@ -98,6 +99,7 @@ export const newCloudShellWorkSpace = (
},
annotations: {
[CLOUD_SHELL_RESTRICTED_ANNOTATION]: 'true',
[CLOUD_SHELL_SOURCE_ANNOTATION]: 'web-terminal',
Copy link
Contributor

Choose a reason for hiding this comment

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

it's going to appear for new instances, but maybe we also should care about existing ones. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there's any mechanism in place to update Web Terminals after creation, and I don't think it's worth adding for this (especially since 4.8 might result in clean installs of WTO).

Copy link
Contributor

Choose a reason for hiding this comment

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

4.8 already happened.
But yeah, we should be good without updating, if we backport it to 4.8 before WTO 1.3

@christianvogt
Copy link
Contributor

/approve
/lgtm

@christianvogt
Copy link
Contributor

No docs necessary.
No changes to user experience.

/label docs-approved
/label px-approved
/label qe-approved

@openshift-ci openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR labels Sep 16, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 16, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, christianvogt, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 16, 2021
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@amisevsk
Copy link
Contributor Author

  • 0x : 0/72 nodes are available: 3 node(s) had taint {node-role.kubernetes.io/infra: }, that the pod didn't tolerate, 3 node(s) had taint {node-role.kubernetes.io/master: }, that the pod didn't tolerate, 66 Insufficient cpu.

😳

/retest

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

5 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 9a449f3 into openshift:master Sep 17, 2021
@amisevsk amisevsk deleted the metrics-annotation-dwo branch September 17, 2021 14:52
@spadgett spadgett added this to the v4.10 milestone Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants