Refactor of system/memory metricset #26334
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
botelastic
bot
added
the
needs_team
|
Pinging @elastic/integrations (Team:Integrations) |
botelastic
bot
removed
the
needs_team
💔 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪Test errors
Expand to view the tests failures
|
| Test | Results |
|---|---|
| Failed | 1 |
| Passed | 18453 |
| Skipped | 1495 |
| Total | 19949 |
Genuine test errors 
💔 There are test failures but not known flaky tests, most likely a genuine test failure.
- Name:
Build&Test / x-pack/metricbeat-pythonIntegTest / test_dashboards – x-pack.metricbeat.tests.system.test_xpack_base.Test
urso
reviewed
Jun 16, 2021
urso
reviewed
Jun 16, 2021
urso
reviewed
Jun 16, 2021
|
/test |
urso
reviewed
Jun 24, 2021
urso
approved these changes
Jun 26, 2021
|
This pull request is now in conflicts. Could you fix it? 🙏 |
|
The three errors that are happening in the E2E are present in that master branch of the project, and all our investigation probably points to the fact that they are caused by #26518. From e2e stand point, this PR is OK, then. |
@mdelapenya thanks for checking this. Could the affected tests be skipped till #26518 is solved? |
|
@jsoriano we are currently discussing about it. We'll let beats contributors mailing list now about the final decision |
jsoriano
approved these changes
Jun 29, 2021
| se_inuse int32 | ||
| se_priority int32 | ||
| sw_path []byte | ||
| } |
There was a problem hiding this comment.
What would happen if a future version of OpenBSD adds attributes to some of these structs? Could they be imported from the C code?
e.g. here it seems they import uvmexp https://github.com/golang/sys/blob/aa57babbf139f2cf189f7cc4dce7ad03fec65ed6/unix/types_openbsd.go#L281
There was a problem hiding this comment.
Agreed, might be worth tinkering with that a bit in the future.
| return memData, errors.Errorf("Error in size VM_UVMEXP sysctl call, errno %d", errno) | ||
| } | ||
|
|
||
| _, _, errno = syscall.Syscall6(syscall.SYS___SYSCTL, uintptr(unsafe.Pointer(&mib[0])), 2, uintptr(unsafe.Pointer(&uvmexp)), uintptr(unsafe.Pointer(&n)), 0, 0) |
There was a problem hiding this comment.
Should n be the maximum between n and sizeof(uvmexp)? Maybe this way it is possible to continue using the current struct even if this is executed on a future kernel that returns more fields?
Same for bcachestats later.
|
@jsoriano thanks for the feedback. I've made a note to fix up the *bsd code in the future, since it's a little deprioritized right now. |
jsoriano
approved these changes
Jun 29, 2021
6 tasks
fearful-symmetry
added a commit
to fearful-symmetry/beats
that referenced
this pull request
Jun 29, 2021
* init commit * start on linux implementation * finish linux, start work on darwin * fix build platform issues * fix metrics on darwin * add openbsd * add freebsd * add windows, aix * fix aix build * finish memory * fix up opt changes * cleanup metricset code * fix up folder methods * fix calculations, Opt API, gomod * make notice * go mod tidy, mage fmt * fix up linux/memory * update fields * update system tests * fix system tests, again * fix extra print statements * fix if block in fillPercentages * vix Value API * fix up tests, opt (cherry picked from commit e008024)
fearful-symmetry
added a commit
that referenced
this pull request
Jun 30, 2021
* Refactor of system/memory metricset (#26334) * init commit * start on linux implementation * finish linux, start work on darwin * fix build platform issues * fix metrics on darwin * add openbsd * add freebsd * add windows, aix * fix aix build * finish memory * fix up opt changes * cleanup metricset code * fix up folder methods * fix calculations, Opt API, gomod * make notice * go mod tidy, mage fmt * fix up linux/memory * update fields * update system tests * fix system tests, again * fix extra print statements * fix if block in fillPercentages * vix Value API * fix up tests, opt (cherry picked from commit e008024) * remove libbeat mem * fix process
mdelapenya
added a commit
to mdelapenya/beats
that referenced
this pull request
Jun 30, 2021
* master: (25 commits) fix: Force PLATFORMS environment variable when we build Elastic Agent dependencies on arm64 (elastic#26415) macos for metricbeat to run in the extended meta-stage (elastic#26573) Packaging: add arm7 platform in the main pipeline (elastic#26575) [Heartbeat] Skip flakey timer queue test (elastic#26592) Update to "read_pipeline" permission (elastic#26465) (elastic#26580) API keys do not reflect the need for read_pipeline (elastic#26466) (elastic#26582) Add Fleet agent.id to Agent monitoring data (elastic#26548) Add kinesis metricset (elastic#25989) Refactor of system/memory metricset (elastic#26334) Introduce httpcommon package in libbeat (add support for Proxy) (elastic#25219) [Filebeat] change multiline configuration in awss3 input to parsers (elastic#25873) docs: Hint for the error "Error extracting container id" (elastic#25824) [Docs] Fixed metricbeat redis exported field CPU descriptions (elastic#25846) (elastic#26496) Update urllib to 1.26.5. (elastic#26380) Update golang.org/x/crypto (elastic#26448) [Filebeat] Update Fortinet Ingest Pipeline (elastic#24816) Move parsers outside of filestream input so others can use them as well (elastic#26541) [Filebeat] Fix `threatintel.indicator.url.full` field not populating (elastic#26508) [Filebeat] Add network direction processor to Zeek and Suricata modules (elastic#24620) Logging code cleanup related to Nomad auto-discovery (elastic#26498) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note that this won't work properly before #26251 is merged and I've re-tested everything with that. However we might also need some additional changes, discussed here: elastic/go-structform#32 (review)
What does this PR do?
This is a continuation of the ongoing refactor of the system module, this time for
system/memory. This is considerably simpler than the changes insystem/cpu, as the data requires less transformation.This is also a first pass at an attempt to move towards more "reporting-defined structures" within the system module, as the main
Memorystruct maps to the final data definition passed upstream. This requires us to do less manual mapping of data, and turns over most of the complexity to the conversion functions. This is designed to work with theOptXtypes introduced in thesystem/cpurefactor, as the above PR togo-structformwill properly see and omit theniltypes.-->
Why is it important?
This is part of an ongoing refactor of the system code, with a few goals
gosigarChecklist
CHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.How to test this PR locally
system/memorymetricset, make sure all fields are reporting properly, and all the data looks reasonable.