-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[processor/k8sattributes] Add container.image.repo_digests
#34031
Conversation
Implementation notes:
|
So eventually we are not going to follow the
from #32314 (comment)? Doesn't this fallback to |
I was trying to simplify this PR by limiting the scope to just populating Including Yes, for theoretical completeness sake, it seems to make sense, but in practice, I don't know whether someone really would use this. Maybe my imagination (or understanding of what value a sha256 ref provides) is too limited. |
@evantorrie sounds good!
The only use-case I would see here is if somebody wants to reference an image even if that's not coming from a repo. For a security analysis, generic issue investigation or sth. But yes it can be done later. |
Unrelated unit test failing due to this: #33998 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good to me based on the prior discussions within the various related issues. Codeowners of the processor should also review/confirm.
@open-telemetry/collector-contrib-approvers Could we get another pair of eyes on this pull request? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@evantorrie the CI failures look unrelated. Consider rebasing as well. |
Direct link to v1.26.0 of semantic conventions Co-authored-by: Christos Markou <chrismarkou92@gmail.com>
@TylerHelmuth @fatsheep9146 This has been pretty much complete for a week or so now. Any chance of getting another review on it from the code-owners? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just one question
Description:
Adds support for extracting canonical repo digest image references for a container from k8s attributes
Link to tracking Issue: #34029
Testing:
.imageID
field from container status API if it is of thereference.Canonical
form, and storing it in aImageRepoDigest
string field. (Note: k8s will only ever return one value here, although the semantic conventions forcontainer.image.repo_digests
define it to be aSlice
. We convert into a Slice lazily when we populate the pcommon data structure.container.image.repo_digests
in rules for extraction.container.image.repo_digests
in the e2e tests for k8sattributes processor.Documentation:
Added
container.image.repo_digests
as an example of attributes that can be extracted from a container