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.

Sign up for GitHub

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

Jump to bottom

container: add CSI attributes #1337

Merged
merged 21 commits into from
Sep 20, 2024
Merged

container: add CSI attributes #1337

merged 21 commits into from
Sep 20, 2024
+42 −8

Conversation

gdvalle
Copy link
Contributor

@gdvalle gdvalle commented Aug 16, 2024 •
edited
Loading

Fixes #1119

Changes

Add CSI (Container Storage Interface) attributes: container.csi.plugin.name and container.csi.volume.id.

Merge requirement checklist

xref open-telemetry/opentelemetry-collector-contrib#32055 (comment)

@gdvalle gdvalle requested review from a team August 16, 2024 16:46
@gdvalle gdvalle mentioned this pull request Aug 16, 2024
Open
jmacd
jmacd requested changes Aug 20, 2024
View reviewed changes
model/registry/k8s.yaml Outdated Show resolved Hide resolved
model/registry/k8s.yaml Outdated Show resolved Hide resolved
gdvalle and others added 2 commits August 21, 2024 14:57
@gdvalle @jmacd
Update model/registry/k8s.yaml
b27dfa5 
Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com>
@gdvalle @jmacd
Update model/registry/k8s.yaml
2069ea8 
Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com>
@gdvalle gdvalle requested a review from jmacd August 21, 2024 19:58
joaopgrassi
joaopgrassi requested changes Aug 22, 2024
View reviewed changes
docs/attributes-registry/k8s.md Outdated Show resolved Hide resolved
@gdvalle gdvalle changed the title k8s: add CSI driver attributes csi: new component Aug 30, 2024
joaopgrassi
joaopgrassi requested changes Sep 2, 2024
View reviewed changes
Copy link
Member

@joaopgrassi joaopgrassi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Make sure to update things via the make targets:

make fix
make generate-gh-issue-templates

model/registry/k8s.yaml Outdated Show resolved Hide resolved
joaopgrassi
joaopgrassi approved these changes Sep 4, 2024
View reviewed changes
docs/attributes-registry/csi.md Outdated Show resolved Hide resolved
@gdvalle
Copy link
Contributor Author

gdvalle commented Sep 4, 2024 •
edited
Loading

I ran into a couple little snags:

@gdvalle gdvalle mentioned this pull request Sep 4, 2024
Merged
@lmolkova
Copy link
Contributor

lmolkova commented Sep 5, 2024

/cc @open-telemetry/semconv-container-approvers @open-telemetry/semconv-k8s-approvers

@gdvalle gdvalle requested review from a team as code owners September 18, 2024 20:47
@gdvalle gdvalle changed the title csi: new component container: add CSI attributes Sep 18, 2024
lmolkova
lmolkova approved these changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

LGTM!

@lmolkova
Copy link
Contributor

@jmacd could you please take another look? I believe your feedback is addressed.

ChrsMark
ChrsMark reviewed Sep 19, 2024
View reviewed changes
model/container/registry.yaml Outdated Show resolved Hide resolved
docs/attributes-registry/csi.md Outdated Show resolved Hide resolved
@lmolkova lmolkova merged commit beaeab8 into open-telemetry:main Sep 20, 2024
13 of 14 checks passed
@gdvalle gdvalle deleted the gtd.csi branch September 20, 2024 18:00
drewby pushed a commit to drewby/otel-semantic-conventions that referenced this pull request Oct 1, 2024
@gdvalle @jmacd
@joaopgrassi @ChrsMark @drewby
container: add CSI attributes (open-telemetry#1337)
a293fde 
Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com>
Co-authored-by: Joao Grassi <5938087+joaopgrassi@users.noreply.github.com>
Co-authored-by: Christos Markou <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Member

ChrsMark commented Oct 18, 2024 •
edited
Loading

Hey @gdvalle, is there a reason why those were only added in the registry and not as Resource Attributes as well?
In https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/32055/files#diff-3343de7bfda986546ce7cb166e641ae88c0b0aecadd016cb253cd5a0463ff464R72-R79 I see they are added as Resource Attributes but from the SemConv perspective that's not clear yet, right? So I wonder if we can actually use them in the implementations right now.

@gdvalle gdvalle mentioned this pull request Oct 21, 2024
Open
@gdvalle
Copy link
Contributor Author

gdvalle commented Oct 21, 2024

Hey @gdvalle, is there a reason why those were only added in the registry and not as Resource Attributes as well? In https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/32055/files#diff-3343de7bfda986546ce7cb166e641ae88c0b0aecadd016cb253cd5a0463ff464R72-R79 I see they are added as Resource Attributes but from the SemConv perspective that's not clear yet, right? So I wonder if we can actually use them in the implementations right now.

No, just my oversight. See #1499 for adding to resource attributes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmolkova lmolkova lmolkova approved these changes

@joaopgrassi joaopgrassi joaopgrassi approved these changes

@jmacd jmacd jmacd approved these changes

@mx-psi mx-psi mx-psi approved these changes

@ChrsMark ChrsMark ChrsMark approved these changes

Assignees

@joaopgrassi joaopgrassi

Labels
None yet
Projects
Semantic Conventions Triage
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

k8s: new attributes: CSI driver and volume handle
6 participants
@gdvalle @lmolkova @ChrsMark @jmacd @mx-psi @joaopgrassi