-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Use alias to report container image in k8s parts #24380
Conversation
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Pinging @elastic/integrations (Team:Integrations) |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@jsoriano @kaiyan-sheng could you please take another look please? #24380 (comment) seems to be valid since code should set the values under ECS fields and not under |
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.
Change looks good to me, do you think it's worth a changelog?
Well if this change is correct it should be transparent to the end users. Otherwise it's a breaking change 😬 ? |
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.
A couple of minor comments but it looks good.
// remove ECS container fields from kubernetes.container.* since they will be set through alias | ||
event.Delete("image") | ||
event.Delete("name") |
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.
Move these deletions to the previous ifs where it is checked if these fields exist?
"meta": common.MapStr{ | ||
"kubernetes": meta, | ||
"kubernetes": meta, // these will be moved to ECS (container.name and container.image.name) through alias type |
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.
This comment looks a bit confusing to me. Not all these kubernetes.*
meta fields are moved, right?
I think that it can be a good idea to add a changelog entry for this change, in the breaking changes section, even if it should be transparent for most users. Even if not recommended, there can be users using custom mappings or no mapping at all and this change can be breaking for them. The entry could say something as " Btw, it could be also breaking in the Metrics UI if it uses the old kubernetes fields and it doesn't handle aliases. We should check this, preferably before merging. @ChrsMark could you confirm this? |
"image": { | ||
"name": "ubuntu:latest" | ||
}, | ||
"name": "ubuntu" |
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.
Sorry, I have just thought on something that can be important. kubernetes.container.name
and container.name
are actually not the same thing, and maybe this was the reason why this field was not filled here.
kubernetes.container.name
is the name of a container in its pod, this is a kubernetes-specific concept and can make sense in a kubernetes-specific field.
container.name
would be the container name in the runtime, that will be probably different, but would be the same concept in multiple orchestrators using the same container runtime.
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.
On the other hand, the image reported by kubernetes.container.image
is the image in the runtime, so it is probably ok if this is moved to a common field.
…name Signed-off-by: chrismark <chrismarkou92@gmail.com>
Long story short, In this, this PR will only apply the change on I updated the description with accordingly (with new screenshots). @kaiyan-sheng @jsoriano sorry for the back-and-forth on this. |
Signed-off-by: chrismark <chrismarkou92@gmail.com>
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.
Thanks!
(cherry picked from commit 85066cb)
What does this PR do?
This PR leverages
type :alias
forkubernetes.container.image
so as to copy the fields under ECScontainer.image.name
.This affects metadata collected by kubernetes autodiscover provider,
add_kubernetes_metadata
processor andstate_container
metricset.Why is it important?
In order to have a persistent way to report ECS container fields, without the need to duplicate fields and store both which will impact ECS.
How to test this PR locally
fields.yml
on your testing env is properly updated with thealias
change and manuallysetup
and verify that mappings are properly loaded in ES.Make sure that
container.image.name
exist in the final documents in ES and you able to search on based on it whilekubernetes.container.image
is not populated in the event but is searchable because of thealias
.3. Check that
add_kubernetes_metadata
processor works:Make sure that
container.image.name
exist in the final documents in ES and you able to search on based on it whilekubernetes.container.image
is not populated in the event but is searchable because of thealias
.state_container
metricset enabled that,container.image.name
exist in the final documents in ES and you able to search based on it whilekubernetes.container.image
is not populated in the event but is searchable because of thealias
.Related issues
Screenshots