Skip to content

[System] Documentation Update: Remove linux-only field mappings from diskio, memory datastr…#7941

Merged
ritalwar merged 4 commits intoelastic:mainfrom
ritalwar:system_docupdate_7935
Sep 27, 2023
Merged

[System] Documentation Update: Remove linux-only field mappings from diskio, memory datastr…#7941
ritalwar merged 4 commits intoelastic:mainfrom
ritalwar:system_docupdate_7935

Conversation

@ritalwar
Copy link
Copy Markdown
Contributor

@ritalwar ritalwar commented Sep 22, 2023 •
edited
Loading

  • Enhancement

What does this PR do?

This PR eliminates field mappings for Linux-specific metrics that were removed from the beats, as seen in this PR and I've used it as a reference to remove the mappings accordingly.
It also revises the documentation to exclude these fields since they are no longer supported, and their presence in our documentation has been causing confusion.

Please note that all of these metrics were duplicated in the Linux module, and users have the option to use Linux integration for the same set of metrics.

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.

Related issues

Screenshots

Screenshot 2023-09-22 at 5 52 05 PM Screenshot 2023-09-22 at 5 52 20 PM

@ritalwar ritalwar requested review from a team as code owners September 22, 2023 12:22
@ritalwar ritalwar added integration Label used for meta issues tracking each integration Integration:system System and removed integration Label used for meta issues tracking each integration labels Sep 22, 2023
@elasticmachine
Copy link
Copy Markdown

elasticmachine commented Sep 22, 2023 •
edited
Loading

💚 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-09-26T13:14:00.091+0000

  • Duration: 16 min 45 sec

Test stats 🧪

Test Results
Failed 0
Passed 151
Skipped 0
Total 151

🤖 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
Copy Markdown

elasticmachine commented Sep 22, 2023 •
edited
Loading

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (3/3) 💚
Files 100.0% (4/4) 💚
Classes 100.0% (4/4) 💚
Methods 63.415% (52/82) ❕
Lines 99.863% (2924/2928) ❕
Conditionals 100.0% (0/0) 💚

shmsr
shmsr requested changes Sep 22, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@shmsr shmsr left a comment

Choose a reason for hiding this comment

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

I haven't done a thorough review yet. For now, left a few comments. I'll try to do a thorough review ASAP.

@ritalwar ritalwar requested a review from shmsr September 22, 2023 15:28
@ishleenk17
Copy link
Copy Markdown
Member

ishleenk17 commented Sep 22, 2023 •
edited
Loading

@ritalwar : In general removal of fields is treated as a breaking change since the customers might have build some viisualizations, and queries on it.
Please see how are such scenarios handled from the customer perspective.

Also, since this is part of a breaking change, this should not be categorized under just a bugfix

@ritalwar
Copy link
Copy Markdown
Contributor Author

ritalwar commented Sep 22, 2023

In general removal of fields is treated as a breaking change since the customers might have build some viisualizations, and queries on it. Please see how are such scenarios handled from the customer perspective.
Also, since this is a breaking change, this should not be categorized under just a bugfix

The breaking change was introduced in the 8.0 version of the Beats system module through this PR. The system integration relies on the beats input system/metrics, and the behavior has remained consistent across integrations. These specific fields have not been supported since version 8.0; however, they were not removed from the integrations and appeared in the exported fields. This situation created confusion for users.
All details are in the PR description and the issue itself. If you have any further questions, feel free to ask.

Additionally, I verified that none of these fields were being used in any visualizations.

@ritalwar ritalwar requested a review from ishleenk17 September 22, 2023 17:55
@shmsr shmsr mentioned this pull request Sep 24, 2023
Closed
ishleenk17
ishleenk17 approved these changes Sep 26, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@ishleenk17 ishleenk17 left a comment

Choose a reason for hiding this comment

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

Looks good!

P1llus
P1llus approved these changes Sep 26, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@P1llus P1llus left a comment

Choose a reason for hiding this comment

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

Just a small nit, but else its fine :)

packages/system/changelog.yml Outdated
@@ -1,4 +1,9 @@
# newer versions go on top
- version: "1.38.3"
Copy link
Copy Markdown
Member

@P1llus P1llus Sep 26, 2023

Choose a reason for hiding this comment

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

Could we bump the minor rather than the hotfix? A bit unsure if this is categorized as a bugfix rather than an enhancement?

Copy link
Copy Markdown
Contributor Author

@ritalwar ritalwar Sep 26, 2023

Choose a reason for hiding this comment

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

@muthu-mps Thoughts?

Copy link
Copy Markdown
Contributor

@muthu-mps muthu-mps Sep 26, 2023

Choose a reason for hiding this comment

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

@P1llus
We have created an internal guidelines on the changelog entries. In which the changes like logo updates, documentation updates etc... would go as a patch release instead of minor release. Still there is a catch when we update the patch it should not be of type: bugfix. Probably for this PR it could be minor bump as we remove the fields from the datastream.

Copy link
Copy Markdown
Contributor Author

@ritalwar ritalwar Sep 26, 2023

Choose a reason for hiding this comment

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

I made a minor version bump and updated its type to "enhancement."

@ritalwar ritalwar changed the title [System] Remove linux-only field mappings from diskio, memory datastr… [System] Documentation Update: Remove linux-only field mappings from diskio, memory datastr… Sep 26, 2023
@ishleenk17
Copy link
Copy Markdown
Member

ishleenk17 commented Sep 26, 2023

Also, since this is part of a breaking change, this should not be categorized under just a bugfix

I agree to @P1llus , as pointed out in my above comment, since this involves the removal of fields, we should not categorize it as a bugfix, but enhancement.

@lalit-satapathy
Copy link
Copy Markdown
Contributor

lalit-satapathy commented Sep 26, 2023

The breaking change was introduced in the 8.0 version of the Beats system module through this elastic/beats#28292.

None of these removed fields, are populated during a Linux run?

lalit-satapathy
lalit-satapathy reviewed Sep 26, 2023
View reviewed changes
packages/system/changelog.yml Outdated
# newer versions go on top
- version: "1.38.3"
changes:
- description: Remove linux-only field mappings from diskio and memory datastreams.
Copy link
Copy Markdown
Contributor

@lalit-satapathy lalit-satapathy Sep 26, 2023

Choose a reason for hiding this comment

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

Should we mention that these fields are not populated and are being removed from the documentation.

Copy link
Copy Markdown
Contributor Author

@ritalwar ritalwar Sep 26, 2023

Choose a reason for hiding this comment

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

updated.

@ritalwar
Copy link
Copy Markdown
Contributor Author

ritalwar commented Sep 27, 2023

The breaking change was introduced in the 8.0 version of the Beats system module through this elastic/beats#28292.

None of these removed fields, are populated during a Linux run?

Yes, there is no data being received for these fields.

@lalit-satapathy
Copy link
Copy Markdown
Contributor

lalit-satapathy commented Sep 27, 2023

LGTM

@ritalwar ritalwar merged commit 59a6583 into elastic:main Sep 27, 2023
@elasticmachine
Copy link
Copy Markdown

elasticmachine commented Sep 27, 2023

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

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

Reviewers

@lalit-satapathy lalit-satapathy lalit-satapathy left review comments

@muthu-mps muthu-mps muthu-mps left review comments

@P1llus P1llus P1llus approved these changes

@shmsr shmsr shmsr approved these changes

@ishleenk17 ishleenk17 ishleenk17 approved these changes

Assignees

No one assigned

Labels

Integration:system System

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Documentation Update: Removal of linux exclusive fields from system integration

7 participants

@ritalwar @elasticmachine @ishleenk17 @lalit-satapathy @P1llus @shmsr @muthu-mps