Bug 1874521: connectivity check metrics cannot be aggregated across components #933

sanchezl · 2020-08-18T02:53:07Z

Implement changes to the metric names and labels as discovered during reviews for better usability.

Names changed to no longer be specific to the component check-endpoints is being run in. They will always be:

pod_network_connectivity_check_count
pod_network_connectivity_check_tcp_connect_latency_gauge
pod_network_connectivity_check_dns_resolve_latency_gauge

endpoint labels renamed to targetEndpoint to avoid collisions with default label of the same name.
component label added (e.g. openshift-kube-apiserver or openshift-apiserver)
checkName label added

sanchezl · 2020-08-18T15:07:51Z

/test verify-deps

sanchezl · 2020-08-19T11:44:45Z

/test images

tkashem · 2020-08-19T12:44:40Z

pod_network_connectivity_check_count
pod_network_connectivity_check_tcp_connect_latency_gauge
pod_network_connectivity_check_dns_resolve_latency_gauge

The metric naming convention followed by k8s is `{namespace}_{sub system}_name
{namespace} : this is the component name, for example: apiserver (kube-apiserver), etcd, kubelet etc.

what do you think about renaming the metrics to connectivity_pod_network_check_*. In future you could have something like connectivity_sdn_* or connectivity_lb_*.

component label added (e.g. openshift-kube-apiserver or openshift-apiserver)

We are already using the label value kube-apiserver to refer to kube-apiserver. I would recommend using the same value to keep it consistent.

tkashem

looks good, just have a couple of minor comments.

tkashem · 2020-08-19T13:13:42Z

pkg/cmd/checkendpoints/controller/metrics.go

+func (m *metricsContext) getCounterMetricLabels(targetEndpoint string, latency *trace.LatencyInfo, checkErr error) map[string]string {
+	labels := m.getMetricLabels(targetEndpoint)
+	labels["dnsResolve"] = ""
+	labels["tcpConnect"] = ""


we can think about choosing a non-empty value to indicate the default/initial condition? In general, it's safer to avoid an empty string - no need to worry about how prometheus handles non existing and empty label.

From https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels:

A label with an empty label value is considered equivalent to a label that does not exist.

sanchezl · 2020-08-20T17:43:24Z

/test images

sanchezl · 2020-08-21T19:41:38Z

The metric naming convention followed by k8s is `{namespace}_{sub system}_name
{namespace} : this is the component name, for example: apiserver (kube-apiserver), etcd, kubelet etc.

what do you think about renaming the metrics to connectivity_pod_network_check_*. In future you could have something like connectivity_sdn_* or connectivity_lb_*.

component label added (e.g. openshift-kube-apiserver or openshift-apiserver)

We are already using the label value kube-apiserver to refer to kube-apiserver. I would recommend using the same value to keep it consistent.

tkashem · 2020-08-21T19:48:28Z

/lgtm

sanchezl · 2020-08-25T17:26:28Z

/test e2e-aws-upgrade

sanchezl · 2020-08-28T01:43:29Z

/test e2e-aws-upgrade

sanchezl · 2020-08-28T13:39:03Z

/test e2e-aws-upgrade

sanchezl · 2020-08-31T18:38:45Z

/test e2e-aws-upgrade

sanchezl · 2020-08-31T19:46:13Z

/test e2e-aws-upgrade

openshift-ci-robot · 2020-09-01T14:29:18Z

@sanchezl: This pull request references Bugzilla bug 1874521, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1874521: connectivity check metrics cannot be aggregated across components

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.

sanchezl · 2020-09-01T14:29:57Z

/bugzilla refresh

openshift-ci-robot · 2020-09-01T14:30:00Z

@sanchezl: This pull request references Bugzilla bug 1874521, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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.

sanchezl · 2020-09-01T14:30:49Z

/bugzilla refresh

openshift-ci-robot · 2020-09-01T14:30:53Z

@sanchezl: This pull request references Bugzilla bug 1874521, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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.

deads2k · 2020-09-01T14:45:05Z

/approve

openshift-ci-robot · 2020-09-01T14:45:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, sanchezl, tkashem

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~~ [deads2k]

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

openshift-bot · 2020-09-01T15:49:04Z

/retest

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

openshift-ci-robot · 2020-09-01T17:52:33Z

@sanchezl: All pull requests linked via external trackers have merged:

openshift/cluster-kube-apiserver-operator#933

Bugzilla bug 1874521 has been moved to the MODIFIED state.

In response to this:

Bug 1874521: connectivity check metrics cannot be aggregated across components

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 requested review from mfojtik and sttts August 18, 2020 02:53

sanchezl force-pushed the check-endpoints-metrics branch from 5fdbb84 to fc3699d Compare August 18, 2020 14:46

check-endpoints: refactor metric names and labels

a47e15b

sanchezl force-pushed the check-endpoints-metrics branch from fc3699d to a47e15b Compare August 19, 2020 02:45

tkashem reviewed Aug 19, 2020

View reviewed changes

sanchezl closed this Aug 21, 2020

sanchezl reopened this Aug 21, 2020

openshift-ci-robot assigned tkashem Aug 21, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2020

sanchezl changed the title ~~check-endpoints: refactor metric names and labels~~ Bug 1874521: connectivity check metrics cannot be aggregated across components Sep 1, 2020

openshift-ci-robot added the bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. label Sep 1, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 1, 2020

openshift-ci-robot removed the bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. label Sep 1, 2020

openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Sep 1, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2020

openshift-merge-robot merged commit fa89e5a into openshift:master Sep 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1874521: connectivity check metrics cannot be aggregated across components #933

Bug 1874521: connectivity check metrics cannot be aggregated across components #933

sanchezl commented Aug 18, 2020 •

edited

sanchezl commented Aug 18, 2020

sanchezl commented Aug 19, 2020

tkashem commented Aug 19, 2020

tkashem left a comment

tkashem Aug 19, 2020

sanchezl Aug 20, 2020

sanchezl commented Aug 20, 2020

sanchezl commented Aug 21, 2020

tkashem commented Aug 21, 2020

sanchezl commented Aug 25, 2020

sanchezl commented Aug 28, 2020

sanchezl commented Aug 28, 2020

sanchezl commented Aug 31, 2020

sanchezl commented Aug 31, 2020

openshift-ci-robot commented Sep 1, 2020

sanchezl commented Sep 1, 2020

openshift-ci-robot commented Sep 1, 2020

sanchezl commented Sep 1, 2020

openshift-ci-robot commented Sep 1, 2020

deads2k commented Sep 1, 2020

openshift-ci-robot commented Sep 1, 2020

openshift-bot commented Sep 1, 2020

openshift-ci-robot commented Sep 1, 2020

Bug 1874521: connectivity check metrics cannot be aggregated across components #933

Bug 1874521: connectivity check metrics cannot be aggregated across components #933

Conversation

sanchezl commented Aug 18, 2020 • edited

sanchezl commented Aug 18, 2020

sanchezl commented Aug 19, 2020

tkashem commented Aug 19, 2020

tkashem left a comment

Choose a reason for hiding this comment

tkashem Aug 19, 2020

Choose a reason for hiding this comment

sanchezl Aug 20, 2020

Choose a reason for hiding this comment

sanchezl commented Aug 20, 2020

sanchezl commented Aug 21, 2020

tkashem commented Aug 21, 2020

sanchezl commented Aug 25, 2020

sanchezl commented Aug 28, 2020

sanchezl commented Aug 28, 2020

sanchezl commented Aug 31, 2020

sanchezl commented Aug 31, 2020

openshift-ci-robot commented Sep 1, 2020

sanchezl commented Sep 1, 2020

openshift-ci-robot commented Sep 1, 2020

sanchezl commented Sep 1, 2020

openshift-ci-robot commented Sep 1, 2020

deads2k commented Sep 1, 2020

openshift-ci-robot commented Sep 1, 2020

openshift-bot commented Sep 1, 2020

openshift-ci-robot commented Sep 1, 2020

sanchezl commented Aug 18, 2020 •

edited