community icon indicating copy to clipboard operation
community copied to clipboard

Published 20 hours ago • verified publisher icon kubernetes

Update instrumentation.md

Open logicalhan opened this issue 1 year ago • 14 comments

Add a snippet about preferring labels to embedding labels in metric names.

Which issue(s) this PR fixes:

Fixes #

logicalhan avatar Feb 07 '24 17:02 logicalhan

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: logicalhan

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

k8s-ci-robot avatar Feb 07 '24 17:02 k8s-ci-robot

/assign @dashpole

logicalhan avatar Feb 07 '24 17:02 logicalhan

Note that this takes the opposite stance taken by OpenMetrics on this question: https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#metric-names-versus-labels

Experience has shown that downstream ingestors find it easier to work with separate total and failure MetricFamiles rather than using {result="success"} and {result="failure"} Labels within one MetricFamily. Also it is usually better to expose separate read & write and send & receive MetricFamiles as full duplex systems are common and downstream ingestors are more likely to care about those values separately than in aggregate.

dashpole avatar Feb 07 '24 17:02 dashpole

I do still prefer what is proposed here. @rexagod @dgrisonnet are you OK with this convention?

dashpole avatar Feb 07 '24 17:02 dashpole

Note that this takes the opposite stance taken by OpenMetrics on this question: https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#metric-names-versus-labels

Experience has shown that downstream ingestors find it easier to work with separate total and failure MetricFamiles rather than using {result="success"} and {result="failure"} Labels within one MetricFamily. Also it is usually better to expose separate read & write and send & receive MetricFamiles as full duplex systems are common and downstream ingestors are more likely to care about those values separately than in aggregate.

Yeah I don't buy it. Ratios are easier with a single metric, because you can do metric["failure or success]" / metric rather than metric1 / sum(metric1 and metric2).

logicalhan avatar Feb 07 '24 18:02 logicalhan

/assign

dgrisonnet avatar Feb 08 '24 17:02 dgrisonnet

Related blog: https://www.robustperception.io/existential-issues-with-metrics/

dashpole avatar Feb 08 '24 17:02 dashpole

Found where the prometheus docs recommend separate failure and total: https://prometheus.io/docs/practices/instrumentation/#failures

dashpole avatar Feb 08 '24 17:02 dashpole

Ratios are easier with a single metric, because you can do metric["failure or success]" / metric rather than metric1 / sum(metric1 and metric2).

It does seem easier to do ratios. However, when evaluating an expression such as metric{result="failure"} / metric in Prometheus, isn't the result going to be a vector with all values equal to 1 because of how Prometheus Vector matching rules work? We could probably rewrite the expression to avoid that, but then is it still easier? This is not what I would expect, but I'm probably misunderstanding something here.

Experience has shown that downstream ingestors find it easier to work with separate total and failure MetricFamiles rather than using {result="success"} and {result="failure"} Labels within one MetricFamily. Also it is usually better to expose separate read & write and send & receive MetricFamiles as full duplex systems are common and downstream ingestors are more likely to care about those values separately than in aggregate.

OpenMetrics authors don't provide much details or examples, which makes it hard (at least for me) to reason/argue. We could try reaching out to them for more information. What do you think?

ricardoapl avatar Feb 08 '24 22:02 ricardoapl

I think Errors, Successes, Totals: Which Metrics Should I Expose to Prometheus? may also be useful for this discussion, in particular Exposing a single metric with a label

ricardoapl avatar Feb 15 '24 17:02 ricardoapl

@juliusv

dashpole avatar Feb 15 '24 17:02 dashpole

Yeah, TL;DR of that article: If you have just have a binary success/failure situation that you want to track, use two different metrics, one for the failures, one for the total (not the successes). If you have something more complicated like many different HTTP response status codes, use a single metric with a label on it instead.

juliusv avatar Feb 15 '24 18:02 juliusv

PR needs rebase.

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.

k8s-ci-robot avatar Apr 04 '24 23:04 k8s-ci-robot

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot avatar Jul 04 '24 00:07 k8s-triage-robot

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle rotten
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot avatar Aug 03 '24 00:08 k8s-triage-robot

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-triage-robot avatar Sep 02 '24 00:09 k8s-triage-robot

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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-sigs/prow repository.

k8s-ci-robot avatar Sep 02 '24 00:09 k8s-ci-robot