[receiver/kubeletstatsreciever] add volume attrs for CSI-backed PVCs#32055
[receiver/kubeletstatsreciever] add volume attrs for CSI-backed PVCs#32055gdvalle wants to merge 13 commits intoopen-telemetry:mainfrom
Conversation
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Bump, not stale, just awaiting review. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Bump |
|
@gdvalle can you fix the merge conflicts and make CI pass? Thanks |
|
@mx-psi CI is green now! |
|
@dmitryax @TylerHelmuth PTAL! |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
TylerHelmuth
left a comment
TylerHelmuth
left a comment
There was a problem hiding this comment.
We've started being stricter with how we add new attributes in k8s components, specifically looking for approval from the semantic convention SIG on the proposed semantic conventions.
Please open a PR at https://github.com/open-telemetry/semantic-conventions, similar to open-telemetry/semantic-conventions#997, to propose these new attributes.
|
@TylerHelmuth or @dmitryax could you please re-open this? The semconv has been established/merged and branch is updated to match. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
| description: "Glusterfs volume path" | ||
| enabled: true | ||
| type: string | ||
| container.csi.volume.id: |
There was a problem hiding this comment.
I'm bit concerned from the semantic conventions perspective since those are not yet defined as resource attributes there. See open-telemetry/semantic-conventions#1337 (comment)
There was a problem hiding this comment.
👍 This needs to go thru semconv first. I'm really not sure if container.csi.volume.id: is a good name.
There was a problem hiding this comment.
Just to avoid possible confusion here. It has been added in SemConv with open-telemetry/semantic-conventions#1337 but is only part of the registry and not defined as Resource or metric attribute. So I'm not sure if we can already use it.
There was a problem hiding this comment.
I opened open-telemetry/semantic-conventions#1499 to clear this up. Is there anything else you can see I'm missing?
| }, | ||
| }, | ||
| { | ||
| name: "persistentVolumeClaim - with detailed PVC labels (CSI PV with GCP driver)", |
There was a problem hiding this comment.
Could we also enhance the scraper's tests like at https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/kubeletstatsreceiver/scraper_test.go#L598? The resulted files are really useful for reference since they contain the whole information.
There was a problem hiding this comment.
Hm, these tests appear to be at least somewhat broken. You can alter anything in the expectedVolumes section of the successful test and it still passes.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
not stale, waiting for consensus in open-telemetry/semantic-conventions#1499 |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
7 participants
A retry of #25170, where it was auto-closed from no maintainer activity.
Description:
Add a couple new attrs to record information about CSI volume type.
These are now part of semconv as of open-telemetry/semantic-conventions#1337
container.csi.plugin.name: The raw CSI.Driver field.container.csi.volume.id: The raw CSI.VolumeHandle field.Also, if we can identify the CSI driver as a GCP-backed one, use the
VolumeHandleto populategce.pd.name. This gives parity with the legacyGCEPersistentDiskin-tree PV kind.Link to tracking Issue: n/a
Testing:
A test is added to validate all labels described above are populated.
Documentation:
A changelog entry is added.