[Metricbeat][Kubernetes] Add container.name field to state_container #37515

constanca-m · 2023-12-27T09:45:58Z

Proposed commit message

Please explain:

WHAT: add container.name field to state_container data stream.
WHY: the field is not being populated as expected right now, issue Field container.name is not populated for kubernetes.state_container integrations#8000

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

Clone this branch.
Follow the steps in the README file.
Check in Discover that the field exists.

Related issues

Closes Field container.name is not populated for kubernetes.state_container integrations#8000

Screenshots

Proof that it works:

mergify · 2023-12-27T09:46:33Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @constanca-m? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

elasticmachine · 2023-12-27T10:18:01Z

❕ Build Aborted

Expand to view the summary

Build stats

Duration: 32 min 32 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2023-12-27T11:00:32Z

💚 Build Succeeded

Expand to view the summary

Build stats

Start Time: 2023-12-27T10:10:48.935+0000
Duration: 50 min 36 sec

Test stats 🧪

Test	Results
Failed	0
Passed	4521
Skipped	902
Total	5423

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

tetianakravchenko

I think this issue needs more clarifications

When I checked this PR I found few relevant PRs/discussions:

container.name was added in Add container image and container name ECS fields for state_container #23802 and removed later Use alias to report container image in k8s parts #24380 in favor of using alias
Also here is this discussion - Verify ECS alignment in container fields reported by kubernetes module and meta processor #23585, see this comment Verify ECS alignment in container fields reported by kubernetes module and meta processor #23585 (comment)
seems that it is not correct just to copy kubernetes.container.name

metricbeat/module/kubernetes/state_container/state_container.go

tetianakravchenko · 2024-01-02T17:55:25Z

metricbeat/module/kubernetes/state_container/state_container.go

+				kubernetes.ShouldPut(containerFields, "name", cName, m.Logger())
+			}
+		}
+
 		// applying ECS to kubernetes.container.id in the form <container.runtime>://<container.id>
 		// copy to ECS fields the kubernetes.container.image, kubernetes.container.name


I think this comment is also applicable to the container.name

I checked, but the value of the field is just <container.name>, it does not have the same format as <container.runtime>://<container.id>.

Thanks for the PR @constanca-m ! I have tested this locally and works fine.

(I am posting the example from my local setup:)

metricset: container -> container.name: nginx
metricset: state_container: -> container.name: nginx

Even works for mutiple containers inside same pod

I think what @tetianakravchenko meant is that the comment copy to ECS fields the kubernetes.container.image, kubernetes.container.name applies also for the addition you did. So it would be better to move your code under this comment.

Co-authored-by: Tetiana Kravchenko <tanya.kravchenko.v@gmail.com>

elasticmachine · 2024-01-03T10:24:20Z

💚 Build Succeeded

Expand to view the summary

Build stats

Start Time: 2024-01-03T09:31:46.900+0000
Duration: 52 min 26 sec

Test stats 🧪

Test	Results
Failed	0
Passed	4521
Skipped	902
Total	5423

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

gsantoro · 2024-01-08T15:17:11Z

@constanca-m have we addressed @tetianakravchenko concerns above about kubernetes.container.name not matching container.name?

Just to summarise this is the comment from Christos #24380 (comment) about those fields having different values.

If they have the same value, we should either use a alias or just keep only one of those like suggested at #23585 (comment).

If they differ, we shouldn't copy so the code changes are not correct either.

Have we verified that those two fields are exactly the same in different runtimes (eg. cloud with all the providers, docker, ...)

gsantoro

verify first that those two fields should have the same values

constanca-m · 2024-01-08T16:10:24Z

I had only tested with docker, but I tested now with GCP, and they are the exact same:

Regarding the comment #23585 (comment),

I don't exactly understand this. @MichaelKatsoulis this comment is 2 years old, but any chance you can give some insight into why container.name is not the same as kubernetes.container.name?

Signed-off-by: constanca <constanca.manteigas@elastic.co>

elasticmachine · 2024-01-09T09:20:51Z

💔 Build Failed

Expand to view the summary

Build stats

Start Time: 2024-01-09T08:44:19.675+0000
Duration: 36 min 25 sec

Test stats 🧪

Test	Results
Failed	0
Passed	3228
Skipped	613
Total	3841

Steps errors

Expand to view the steps failures

`metricbeat-windows-2022-windows-2022 - Install Python`

Took 0 min 5 sec . View more details here
Description: .ci/scripts/install-tools.bat

`metricbeat-windows-2022-windows-2022 - Install Python`

Took 0 min 2 sec . View more details here
Description: .ci/scripts/install-tools.bat

`metricbeat-windows-2022-windows-2022 - Install Python`

Took 0 min 4 sec . View more details here
Description: .ci/scripts/install-tools.bat

`x-pack/metricbeat-windows-2022-windows-2022 - Install Python`

Took 0 min 4 sec . View more details here
Description: .ci/scripts/install-tools.bat

`x-pack/metricbeat-windows-2022-windows-2022 - Install Python`

Took 0 min 2 sec . View more details here
Description: .ci/scripts/install-tools.bat

`x-pack/metricbeat-windows-2022-windows-2022 - Install Python`

Took 0 min 4 sec . View more details here
Description: .ci/scripts/install-tools.bat

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2024-01-09T10:58:42Z

❕ Build Aborted

Expand to view the summary

Build stats

Duration: 6 min 22 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

MichaelKatsoulis · 2024-01-09T11:38:18Z

They are the same because in your code you get the container.name value from the event, which is the one added by ksm. So they cannot be different.
The only case they can differ is in docker runtime as I have seen. Docker names the containers differently than the name that is set in the manifest. You can see that by running docker ps.
But we don't seem to populate the container.name field anywhere other than the kubernetes container metricset. And even there, we set it to the value of the event.

I think that the problem is that the concepts of those two fields is a bit different and shouldn't be aliased. Now regarding setting it explicitly in the state_container as we do in container metricset I am not sure.
ECS fields, supposedly could be populated by other sources such as Otel. So we should be very cautious when populating them.
@ChrsMark what do you think on this as you have been working with the introduction of ECS fields to Otel community?

elasticmachine · 2024-01-09T11:44:44Z

💚 Build Succeeded

Expand to view the summary

Build stats

Start Time: 2024-01-09T10:53:57.165+0000
Duration: 51 min 40 sec

Test stats 🧪

Test	Results
Failed	0
Passed	4549
Skipped	902
Total	5451

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

ChrsMark · 2024-01-09T14:41:14Z

SemConv of OTel for container resource specifically defines:

Container name used by container runtime

(The same, conceptually, defines ECS as well)

So the point here is that container.name should only be populated with values coming from the runtime (whatever it is).

OTel also specifies the k8s.container.name attribute: https://opentelemetry.io/docs/specs/semconv/resource/k8s/#container

This implies that these 2 can be different in principle.

There might be cases that runtimes's names match what we see in the spec but we should not overload container.name with values coming from the Pod spec if we are not sure what the container runtime gives.

Example: Let's say that we have Metricbeat collecting these data and we do container.name<=kubernetes.container.name.
Then if another low level agent (a security scanning mechanism for example) runs on the nodes and interacts with the container runtimes directly then there is a chance that we will have a missmatch there and will not be able to correlate the data.

I haven't done any extensive research on this recently so I might miss sth here but I think that's how we should approach this one here.

MichaelKatsoulis · 2024-01-10T07:56:20Z

So after @ChrsMark comment, I understand that is wrong to assume that container.name equals to kubernetes.container.name and if we are not sure of the runtime container name, we should not populate this field.

I believe the best approach is to not populate this field at all and also remove it from the container metricset.
cc @gizas

constanca-m · 2024-01-10T09:18:55Z

Ok, seeing this implementation is wrong, I will close this PR and the issue.

Add container.name field.

32aace0

constanca-m added bug Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team labels Dec 27, 2023

constanca-m self-assigned this Dec 27, 2023

constanca-m requested a review from a team as a code owner December 27, 2023 09:45

constanca-m requested review from gsantoro and tetianakravchenko December 27, 2023 09:45

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Dec 27, 2023

constanca-m mentioned this pull request Dec 27, 2023

Field container.name is not populated for kubernetes.state_container elastic/integrations#8000

Closed

run go test -data

6d33a62

constanca-m changed the title ~~[Kubernetes] Add container.name field to state_container~~ [Metricbeat][Kubernetes] Add container.name field to state_container Dec 27, 2023

tetianakravchenko requested changes Jan 2, 2024

View reviewed changes

Update metricbeat/module/kubernetes/state_container/state_container.go

3526459

Co-authored-by: Tetiana Kravchenko <tanya.kravchenko.v@gmail.com>

gizas approved these changes Jan 8, 2024

View reviewed changes

constanca-m requested a review from tetianakravchenko January 8, 2024 15:06

gsantoro requested changes Jan 8, 2024

View reviewed changes

MichaelKatsoulis self-requested a review January 9, 2024 08:01

Move code

222bc3d

Signed-off-by: constanca <constanca.manteigas@elastic.co>

Merge branch 'main' into add-container-name

5702cea

constanca-m closed this Jan 10, 2024

constanca-m deleted the add-container-name branch May 17, 2024 13:18

[Metricbeat][Kubernetes] Add container.name field to state_container #37515

[Metricbeat][Kubernetes] Add container.name field to state_container #37515

Conversation

constanca-m commented Dec 27, 2023

Proposed commit message

Checklist

How to test this PR locally

Related issues

Screenshots

mergify bot commented Dec 27, 2023

elasticmachine commented Dec 27, 2023

❕ Build Aborted

Build stats

🤖 GitHub comments

elasticmachine commented Dec 27, 2023

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

tetianakravchenko left a comment • edited Loading

Choose a reason for hiding this comment

tetianakravchenko Jan 2, 2024

Choose a reason for hiding this comment

constanca-m Jan 3, 2024

Choose a reason for hiding this comment

gizas Jan 8, 2024

Choose a reason for hiding this comment

MichaelKatsoulis Jan 9, 2024

Choose a reason for hiding this comment

elasticmachine commented Jan 3, 2024

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

gsantoro commented Jan 8, 2024 • edited Loading

gsantoro left a comment

Choose a reason for hiding this comment

constanca-m commented Jan 8, 2024

elasticmachine commented Jan 9, 2024

💔 Build Failed

Build stats

Test stats 🧪

Steps errors

metricbeat-windows-2022-windows-2022 - Install Python

metricbeat-windows-2022-windows-2022 - Install Python

metricbeat-windows-2022-windows-2022 - Install Python

x-pack/metricbeat-windows-2022-windows-2022 - Install Python

x-pack/metricbeat-windows-2022-windows-2022 - Install Python

x-pack/metricbeat-windows-2022-windows-2022 - Install Python

💚 Flaky test report

🤖 GitHub comments

elasticmachine commented Jan 9, 2024

❕ Build Aborted

Build stats

🤖 GitHub comments

MichaelKatsoulis commented Jan 9, 2024

elasticmachine commented Jan 9, 2024

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

ChrsMark commented Jan 9, 2024

MichaelKatsoulis commented Jan 10, 2024

constanca-m commented Jan 10, 2024

tetianakravchenko left a comment •

edited

Loading

gsantoro commented Jan 8, 2024 •

edited

Loading

`metricbeat-windows-2022-windows-2022 - Install Python`

`metricbeat-windows-2022-windows-2022 - Install Python`

`metricbeat-windows-2022-windows-2022 - Install Python`

`x-pack/metricbeat-windows-2022-windows-2022 - Install Python`

`x-pack/metricbeat-windows-2022-windows-2022 - Install Python`

`x-pack/metricbeat-windows-2022-windows-2022 - Install Python`