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

System breaking changes for 8.0 #28292

Merged

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented Oct 6, 2021

What does this PR do?

This PR makes a series of long-awaited breaking changes in the system module in preparation for 8.0

Notably, it removes a series of linux-exclusive metrics currently in the system module. These changes have been in place in elastic-agent for nearly a year, but for the sake of backwards compatibility, I added a series of shims so the metrics remained in metricbeat until 8.0.

Note that all these metrics are duplicated in the linux module, and these shims were to keep duplicated metrics in the system module, where they had lived before we decided to "clean up" the system module to remove the (at the time) surplus of linux-only metrics.

Why is it important?

This removes a bit of tech debt that was put in place to avoid making breaking changes.

Checklist

  • I have made corresponding changes to the documentation
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@fearful-symmetry fearful-symmetry added enhancement Team:Integrations Label for the Integrations team backport-skip Skip notification from the automated backport with mergify labels Oct 6, 2021
@fearful-symmetry fearful-symmetry requested a review from a team October 6, 2021 21:55
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic 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 Oct 6, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Oct 6, 2021

💚 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

  • Duration: 94 min 36 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 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.

@mergify
Copy link
Contributor

mergify bot commented Oct 11, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b system-breaking-changes upstream/system-breaking-changes
git merge upstream/master
git push upstream system-breaking-changes

Copy link
Contributor

@narph narph left a comment

Choose a reason for hiding this comment

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

Glad to see this is happening, you mention these metrics are already in the linux module, I was wondering if the removal affects any system visualizations or if we have been warning our linux users about the move? Do we have any deprecation message in place for now?

@fearful-symmetry
Copy link
Contributor Author

I was wondering if the removal affects any system visualizations

Last I checked, these are independent of any visualizations

Do we have any deprecation message in place for now?
@narph do you know where that would go?

@narph
Copy link
Contributor

narph commented Oct 13, 2021

Do we have any deprecation message in place for now?
@narph do you know where that would go?

we usually add a cfgwarn.Deprecate message depending on the case.
Maybe when initializing the metricset telling users they will have to use the linux module in the future in case of those metrics.
@masci, is there any process in place for deprecating/removing metrics to make the transition easier for users?
@jsoriano do you know if we had any precedent for this case?

@masci
Copy link

masci commented Oct 13, 2021

Looping in @akshay-saraswat and @mukeshelastic to help us making this call

@jsoriano
Copy link
Member

we usually add a cfgwarn.Deprecate message depending on the case.
Maybe when initializing the metricset telling users they will have to use the linux module in the future in case of those metrics.

I think this should have been done when the linux module was added and the metrics in the system module were deprecated. Now that this code is being removed, there is no place where to place this warning 😅

These are metricsets enabled by default, so I guess users using default config are not going to miss any data, but we have to check the migration paths. What is going to happen to linux users when they upgrade to 8.0 if they have customized the system.yml?

@fearful-symmetry
Copy link
Contributor Author

What is going to happen to linux users when they upgrade to 8.0 if they have customized the system.yml?

@jsoriano this is a tad awkward, since we're not really depricating any metricsets or config values, but chunks of reported metrics within those metricsets. From a user config standpoint, nothing is really changing.

@narph
Copy link
Contributor

narph commented Oct 14, 2021

@jsoriano

I think this should have been done when the linux module was added and the metrics in the system module were deprecated. Now that this code is being removed, there is no place where to place this warning 😅

I agree here, we should have introduced the message sooner, the metricset is not being removed just several metrics so I was asking if it makes sense to add the deprecation message still in 7.16 and/or 8.0 (different wording in 8.0). Users might be caught off-guard when upgrading to 8.0 and not finding those metrics anymore in the system module.

@fearful-symmetry
Copy link
Contributor Author

@narph / @jsoriano so, we should merge this, and open a separate PR for adding the deprecation warning?

@narph
Copy link
Contributor

narph commented Oct 15, 2021

@narph / @jsoriano so, we should merge this, and open a separate PR for adding the deprecation warning?

I am good with that

@fearful-symmetry fearful-symmetry merged commit c48a2f4 into elastic:master Oct 15, 2021
Icedroid pushed a commit to Icedroid/beats that referenced this pull request Nov 1, 2021
* make breaking changes for 8.0

* changelog

* update

* fix tests

* add removed build tag

* trying to fix windows tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip notification from the automated backport with mergify enhancement Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants