Bug 1845873: Add rhmi_status metric to telemeter

[david-martin]

New metric to be whitelisted for telemeter

• rhmi_status

Used with OCM to inform of RHMI installation progress.
The total number of series/cardinality is 8 (based on their being 8 possible stages)

[david-martin]

/cc @alanmoran @maleck13 @jjaferson

[openshift-ci-robot]

@david-martin: GitHub didn't allow me to request PR reviews from the following users: maleck13.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @alanmoran @maleck13 @jjaferson

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[lilic]

Couple of questions:

• What is the total number of series you will be sending for each component, as each team gets a budget?
• Did you get Claytons 👍 on this, as per the docs? :)
• If I remember correctly you are not installed by default in every openshift cluster, so this makes it hard to test things?

Also note this can only happen from 4.6 onwards, you are targeting master, but master - 4.5 is closed for these kind of changes so you have to wait until 4.6 master opens.

[david-martin]

• What is the total number of series you will be sending for each component, as each team gets a budget?

• rhmi_status => only every 1 instance of this metric per cluster (labels may change over time, but still just 1 instance)
• rhmi_version => only every 1 instance of this metric per cluster (labels may change over time, but still just 1 instance)

• Did you get Claytons 👍 on this, as per the docs? :)

Please link the docs where this is mentioned in case I've missed anything else.

• If I remember correctly you are not installed by default in every openshift cluster, so this makes it hard to test things?

Yes.
Enabling the 'rhmi' addon will be enough to start scraping the metrics.
Let me know what can be done here to satisfy testing criteria

[lilic]

The docs are linked in both forum-telemetry and forum-monitoring :) https://docs.google.com/document/d/1a6n5iBGM2QaIQRg9Lw4-Npj6QY9--Hpx3XYut-BrUSY/edit

(labels may change over time, but still just 1 instance)

You mean label value or label name?

Might be easier to link to the metrics in code, thanks!

[david-martin]

The docs are linked in both forum-telemetry and forum-monitoring

Thanks, I was only looking in this repo.
Reviewing now.

You mean label value or label name?
Label value

Might be easier to link to the metrics in code, thanks

See both metrics in https://github.com/integr8ly/integreatly-operator/blob/master/pkg/metrics/metrics.go

[lilic]

https://github.com/integr8ly/integreatly-operator/blob/master/pkg/metrics/metrics.go#L66-L72

Seems like its much more than 1 series here, I have not looked at the various label values you can set, but the following makes me think these are of unbound high cardinality metrics. https://github.com/integr8ly/integreatly-operator/blob/5cdc436d52c04f18e7cc56644b3f1a44060676af/pkg/controller/installation/installation_controller.go#L710

As per the document, we are trying to limit the number of series each component to 1 - 10 series for now. If you get special approval from Clayton on more, thats fine by me. But this looks like you are going to be sending unbound number of series at least for this one metric, did not look at others.

Let me know if there is something that I am missing or anything unclear.

[david-martin]

As per the document, we are trying to limit the number of series each component to 1 - 10 series for now

Thanks, this is starting to make more sense to me now.
I'll review in more detail and get a better idea of cardinality, and suggest changes to get that value within the range.

[lilic]

Sounds good to me, feel free to rechout to me or any other team members on forum-monitoring if you have any other questions!

[vkareh]

Could you please also add @openshift/openshift-team-cluster-manager in here for change notifications, as this metric will be consumed by OCM (for self-service RHMI)

[vkareh]

Thanks! 👍

[david-martin]

/hold while metrics are under review

[kbsingh]

@lilic these specific pieces should land part of middleware budget

[s-urbaniak]

/assign pgier

[s-urbaniak]

@pgier do you mind to have a look at this one?

[s-urbaniak]

@david-martin please let us know once your cardinality investigation is done. Do you mind to share the results (numbers) simply here? Thanks! :-)

[lilic]

@s-urbaniak there is nothing to review yet, I have already reviewed the document of the new proposed metrics, I can take it from here as I know the background:)

[lilic]

@kbsingh was not aware that is a different budget, they still go to telemtry so similar rules apply? But as discussed these metrics were way over any budget as they were unbound cardinality.

[david-martin]

@lilic I've updated the PR to only include the rhmi_status metric.
The PR in the RHMI Operator for this metric is integr8ly/integreatly-operator#857

Example of the metric series exposed:

EDIT: Now only exposes the current stage (No 0 values)

# HELP rhmi_status RHMI status of an installation
# TYPE rhmi_status gauge
rhmi_status{stage="complete"} 1

Let me know if there's anything else I need to do here to get this merged.

[david-martin]

/unhold

@lilic I believe this is ready for another review.
Let me know if anything else needs to be done.
The e2e-aws CI failure looks unrelated to this change

@vkareh What's the right process here to get this backported for a 4.4 z-stream release as well?

[lilic]

@david-martin

Let me know if anything else needs to be done.

These look good from my perspective, thanks! Last thing is did you get an okay from Clayton on the metrics as well?

[vkareh]

@david-martin

What's the right process here to get this backported for a 4.4 z-stream release as well?

Sorry, I don't know this process - I work on the OCM bits, my project is just a consumer

[lilic]

You need a valid bugzilla medium or higher priority for backports, this should be against the RHMI component in bugzilla.

[s-urbaniak]

@vkareh as @lilic mentioned you need a BZ against the owning component, but according to @david-martin there seems to be no component owner in OpenShift for RHMI metrics?

pinging @bparees @smarterclayton to clarify which component owns RHMI in BZ, this seems to be a black-hole situation if we ship metrics in OpenShift who are not owned by anybody.

[bparees]

pinging @bparees @smarterclayton to clarify which component owns RHMI in BZ, this seems to be a black-hole situation if we ship metrics in OpenShift who are not owned by anybody.

maybe we can use the install component for now, but @sdodson might have better ideas or want to push for a new component(or sub-component) to be added to cover this.

[smarterclayton]

Can you be in multiple stages? If not, why not only report a single series?

[david-martin]

Can you be in multiple stages?

No.

If not, why not only report a single series?

I may be misunderstanding how labels impact the number of series, but do you mean only ever expose 1 metric result from the scrape endpoint with whatever the current stage is in the stage label (with a value of 1)?

e.g.

• from 11:30 to 11:35 rhmi_status{stage="bootstrap"} 1
• from 11:35 to 11:39 rhmi_status{stage="products"} 1
• after 11:39 rhmi_status{stage="complete"} 1

Or use the value to represent the stage (no label)?

e.g.

• from 11:30 to 11:35 rhmi_status{} 1
• from 11:35 to 11:39 rhmi_status{} 2
• after 11:39 rhmi_status{} 3

[smarterclayton]

The first one - you don't need 8 series with 1 set to 1, and the other 7 set to 0, if the question you are primarily answering is "how many clusters are in this state", "is this cluster in this state". The zero values don't convey any additional info over their absence as far as I can tell

    from 11:30 to 11:35 rhmi_status{stage="bootstrap"} 1
    from 11:35 to 11:39 rhmi_status{stage="products"} 1
    after 11:39 rhmi_status{stage="complete"} 1

[david-martin]

you don't need 8 series with 1 set to 1

Good point.
My thinking was trying to mimic what kube-state-metrics does for some metrics like kube_pod_status_phase e.g.

kube_pod_status_phase{...,phase="Failed",...} | 0
kube_pod_status_phase{...,phase="Pending",...} | 0
kube_pod_status_phase{...,phase="Running",...} | 1
kube_pod_status_phase{...,phase="Succeeded",...} | 0
kube_pod_status_phase{...,phase="Unknown",...} | 0

I'll update the metric upstream in integr8ly/integreatly-operator#857 to omit the 0's.

[lilic]

True, if 0 values you don't care about, you could do a recording rule that checks anything larger than 0 or == 1, and just send the recording rule via telemetry, that will indeed save you few series per cluster.

[david-martin]

Confirming I've updated the metric being exposed so it doesn't return 0 values (integr8ly/integreatly-operator#857)

[sdodson]

maybe we can use the install component for now, but @sdodson might have better ideas or want to push for a new component(or sub-component) to be added to cover this.

NACK on Installer but I really don't have a better suggestion. Ideally there'd be a component in the repo's OWNERS file which could be used to discover. If we only care about appeasing the bot for backport just stick it in Unknown but make sure it's clear that it doesn't need to be triaged to another component.

[lilic]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: david-martin, lilic

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:

• OWNERS [lilic]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[lilic]

/retest

[openshift-bot]

/retest

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

[openshift-bot]

/retest

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

[david-martin]

/cherrypick release-4.5

[openshift-cherrypick-robot]

@david-martin: #795 failed to apply on top of branch "release-4.5":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	Documentation/data-collection.md
M	Documentation/sample-metrics.md
M	Documentation/telemeter_query
M	manifests/0000_50_cluster_monitoring_operator_04-config.yaml
Falling back to patching base and 3-way merge...
Auto-merging manifests/0000_50_cluster_monitoring_operator_04-config.yaml
CONFLICT (content): Merge conflict in manifests/0000_50_cluster_monitoring_operator_04-config.yaml
Auto-merging Documentation/telemeter_query
CONFLICT (content): Merge conflict in Documentation/telemeter_query
Auto-merging Documentation/sample-metrics.md
CONFLICT (content): Merge conflict in Documentation/sample-metrics.md
Auto-merging Documentation/data-collection.md
CONFLICT (content): Merge conflict in Documentation/data-collection.md
Patch failed at 0001 Add RHMI metrics to telemeter

In response to this:

/cherrypick release-4.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci-robot]

@david-martin: Bugzilla bug 1845873 is in an unrecognized state (VERIFIED) and will not be moved to the MODIFIED state.

In response to this:

Bug 1845873: Add rhmi_status metric to telemeter

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.