Skip to content
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

Add metrics for deployment, replicaset, replication_controller, statefulset and hpa #1636

Merged
merged 13 commits into from
Dec 17, 2024

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Nov 29, 2024

Changes

Part of #1032.

Fixes #1644
Fixes #1637

Adds the following metrics:

These metrics have been in use by the Opentelemetry Collector and specifically the k8scluster receiver component.

Breaking changes between the Collector and the Semconv will be listed in the k8s migration guide once #1636 is merged.

Merge requirement checklist

@ChrsMark
Copy link
Member Author

ChrsMark commented Dec 3, 2024

@open-telemetry/specs-semconv-approvers should the .count suffix be applied here based on the guideline from

#### Use `count` Instead of Pluralization for UpDownCounters
?

We don't use a pluralized metric name here but I want to double check if this rule should be taken into account.

It would be preferable if we define this metrics as is to minimize breaking changes of the Collector.

@ChrsMark ChrsMark force-pushed the add_deploy_replica_metrics branch from 07e6fd4 to eee691b Compare December 3, 2024 12:55
model/k8s/metrics.yaml Show resolved Hide resolved
model/k8s/metrics.yaml Outdated Show resolved Hide resolved
docs/system/k8s-metrics.md Outdated Show resolved Hide resolved
@ChrsMark ChrsMark mentioned this pull request Dec 3, 2024
@ChrsMark ChrsMark changed the title Add desired and available metrics for deployment and replicaset Add desired and available metrics for deployment, replicaset and replication_controller Dec 3, 2024
@ChrsMark
Copy link
Member Author

ChrsMark commented Dec 3, 2024

I added the replication_controller metrics in this PR since the names are already the same and group well here.

In any case, we need to have this aligned with #1637 and #1644.

@dmitryax dmitryax self-requested a review December 3, 2024 21:02
@dmitryax
Copy link
Member

dmitryax commented Dec 3, 2024

Taking back my approval until discussion in #1637 (comment) is resolved

@ChrsMark ChrsMark marked this pull request as draft December 3, 2024 21:31
@ChrsMark
Copy link
Member Author

ChrsMark commented Dec 4, 2024

I have updated the PR to include the statefulset and hpa metrics since those are related to the consistency discussion (#1637 (comment)).

If want to achieve consistency while also being semantically correct we either need to use the _pods or the _replicas suffix. I chose to use the _pods suffix as more "user friendly".

Providing bellow the current state (copied from the PR's description):

Breaking changes between the Collector and the Semconv will be listed in the k8s migration guide once #1636 is merged.

@ChrsMark ChrsMark marked this pull request as ready for review December 4, 2024 11:03
@ChrsMark ChrsMark force-pushed the add_deploy_replica_metrics branch from bc46f76 to ce46ae4 Compare December 4, 2024 11:20
@ChrsMark ChrsMark changed the title Add desired and available metrics for deployment, replicaset and replication_controller Add metrics for deployment, replicaset, replication_controller, statefulset and hpa Dec 4, 2024
model/k8s/metrics.yaml Outdated Show resolved Hide resolved
model/k8s/metrics.yaml Outdated Show resolved Hide resolved
model/k8s/metrics.yaml Outdated Show resolved Hide resolved
model/k8s/metrics.yaml Outdated Show resolved Hide resolved
docs/system/k8s-metrics.md Outdated Show resolved Hide resolved
@jinja2
Copy link

jinja2 commented Dec 11, 2024

We have a few minor discrepancies in resource/metric names. While I don't think we need to change any existing names, I would like to note them down for future reference.

  1. Short/Full names for k8s type - HPA seems to be the only resource type that uses the short name. The full object kind can be a mouthful, but best to stick with the full names as long as they are a reasonable length.
  2. k8s object kinds use PascalCase. Some resources mirror this with underscores in the attr/metric names (e.g. k8s.replication_controller for k8s obj ReplicationController), while others don't (e.g. k8s.statefulset for k8s obj StatefulSet). I think we should be consistent with the underscore convention.

@jinja2
Copy link

jinja2 commented Dec 11, 2024

I think this observation from @dashpole might be relevant to some of the metrics here. Most of these timeseries seem meaningfully additive

@ChrsMark
Copy link
Member Author

I think this observation from @dashpole might be relevant to some of the metrics here. Most of these timeseries seem meaningfully additive

As discussed in the K8s SemConv WG meeting today, we agree on making the change to updowncounter for these metrics (I think this will also apply to #1649). I will update the PR(s) to reflect this.

@ChrsMark ChrsMark force-pushed the add_deploy_replica_metrics branch 2 times, most recently from 15d8278 to dbb71af Compare December 12, 2024 08:58
Copy link

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

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

I appreciate that we moved from repliaca -> pods for the convention here. I think that change will make the user experience a bit simpler as well. Thank you!

@ChrsMark
Copy link
Member Author

@open-telemetry/specs-semconv-maintainers could you also take a look please?

note: I'm suggesting we introduce the metrics as-is and follow-up for enhancing their descriptions more to cover their specific relations with resources.

@ChrsMark ChrsMark mentioned this pull request Dec 13, 2024
3 tasks
ChrsMark and others added 11 commits December 14, 2024 16:06
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Co-authored-by: Tetiana Kravchenko <tanya.kravchenko.v@gmail.com>
Co-authored-by: Tetiana Kravchenko <tanya.kravchenko.v@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark force-pushed the add_deploy_replica_metrics branch from 0beb344 to 1f91666 Compare December 14, 2024 14:07
@ChrsMark
Copy link
Member Author

I added resource correlation note per metric with 1f91666 according to the discussion at #1649.

@jsuereth jsuereth enabled auto-merge (squash) December 17, 2024 15:27
@jsuereth jsuereth merged commit b60cdba into open-telemetry:main Dec 17, 2024
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

[k8s] Add hpa metrics Add metrics for k8s statefulset
10 participants