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.

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

[Kubernetes] Reference ECS fields and add agent.id field #8697

Merged

Conversation

constanca-m
Copy link
Contributor

@constanca-m constanca-m commented Dec 12, 2023

What and why

Changes of this PR:

  1. Reference ECS fields. This comes from this comment reference a similar issue [system] - Cleanup fields.yml files #8100.
  2. Add agent.id as dimension to every TSDB field, as it is one of the required fields (reference).

Warning: Running elastic-package format changed some of the files, but there are no differences in those apart from that.

Fields changed to ECS in agent.yml:

  • cloud.account.id
  • cloud.availability_zone
  • cloud.instance.id
  • cloud.instance.name
  • cloud.machine.type
  • cloud.provider
  • cloud.region
  • cloud.project.id
  • host.architecture
  • host.domain
  • host.hostname
  • host.id
  • host.ip
  • host.mac
  • host.name
  • host.os.family
  • host.os.kernel
  • host.os.name
  • host.os.platform
  • host.os.version
  • host.type
  • container.id
  • container.image.name
  • container.labels
  • container.name

Fields changed to ECS in base-fields.yml:

  • data_stream.type
  • data_stream.dataset
  • data_stream.namespace
  • event.dataset
  • '@timestamp'
  • log.file.path - for logs data streams

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Results

Nothing should change.

I checked the dashboard of every data stream and they all work as before.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
@constanca-m constanca-m self-assigned this Dec 12, 2023
@constanca-m constanca-m requested a review from a team as a code owner December 12, 2023 10:24
@constanca-m constanca-m added the Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team label Dec 12, 2023
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
@elasticmachine
Copy link

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@elasticmachine
Copy link

elasticmachine commented Dec 12, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-12-12T12:30:23.255+0000

  • Duration: 60 min 57 sec

Test stats 🧪

Test Results
Failed 0
Passed 97
Skipped 0
Total 97

🤖 GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

@elasticmachine
Copy link

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (1/1) 💚
Files 100.0% (1/1) 💚
Classes 100.0% (1/1) 💚
Methods 96.386% (80/83)
Lines 100.0% (22/22) 💚
Conditionals 100.0% (0/0) 💚

@constanca-m
Copy link
Contributor Author

/test

@MichaelKatsoulis
Copy link
Contributor

The purpose of ecs.yml file in each data stream as I understand is to declare in it all the ecs fields. Does it make sense to have such a file and also have ecs fields declared in other files like base-fields.yml and agent.yml.
This happens also in other packages but I don't see why.

@constanca-m
Copy link
Contributor Author

Does it make sense to have such a file and also have ecs fields declared in other files like base-fields.yml and agent.yml.

I noticed it too, but I also don't know what was the logic followed to split all the fields between the files. Maybe moving them belongs to a different PR @MichaelKatsoulis

@MichaelKatsoulis
Copy link
Contributor

Does it make sense to have such a file and also have ecs fields declared in other files like base-fields.yml and agent.yml.

I noticed it too, but I also don't know what was the logic followed to split all the fields between the files. Maybe moving them belongs to a different PR @MichaelKatsoulis

Yes. It seems that nobody is following the recommendations in https://github.com/elastic/integrations/blob/main/docs/fine_tune_integration.md (bullet 5).
I believe:

  1. ecs.yml should contain all ecs fields of the data stream
  2. agent.yml should contain all fields that are added by agent and are not ecs compliant (host.containerized for example)
  3. base-fields.yml or package-fields.yml should contain all fields added by the package but not from the specific data stream and are not ecs compliant
  4. fields.yml should contain all data stream specific fields

But yes this belongs to a different PR and honestly I don't know if it worths the trouble. @gizas , @axw what do you think ?

@constanca-m could you add in the description of this PR all the field names that you changed to ECS, so that we have them in one place?

@axw
Copy link
Member

axw commented Dec 14, 2023

But yes this belongs to a different PR and honestly I don't know if it worths the trouble. @gizas , @axw what do you think ?

I don't know why some ECS fields are in base-fields.yml -- we also do that in the APM integration package. I'd probably just leave it, I don't think it really matters much.

@constanca-m constanca-m merged commit 6a42944 into elastic:main Dec 14, 2023
4 checks passed
@constanca-m constanca-m deleted the kubernetes-remove-external-fields branch December 14, 2023 10:47
@elasticmachine
Copy link

Package kubernetes - 1.55.1 containing this change is available at https://epr.elastic.co/search?package=kubernetes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Integration:kubernetes Kubernetes Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants