Refactor Multi Node Analyzers

[diamonwiggins]

Description, Motivation and Context

Refactoring analyzers to be easier to write. All analyzers implement a function to check a condition and then return true/false. All analyzers also evaluate outcomes in a similar way.

The recently added code to check if there are contents from remote collection present and then proceed to use them and if not fallback to local contents is also code that can be shared amongst all analyzers.

The goal is that these changes makes it easier to update all remaining analyzers to be able to process remote collected contents, and also just easier to maintain and update host analyzers going forward.

TODO:

• Make sure logging is consistent with rest of codebase
• Update/Fix Tests

Checklist

• New and existing tests pass locally with introduced changes.
• Tests for the changes have been added (for bug fixes / features)
• The commit message(s) are informative and highlight any breaking changes
• Any documentation required has been added/updated. For changes to https://troubleshoot.sh/ create a PR here

Does this PR introduce a breaking change?

• Yes
• No

[nvanthao]

The code changes looks good, I just have few comments and questions.

Understand that all other analyzers will have to use function retrieveCollectedContents to retrieve the files before analyzing.

We will have to ensure that other host collectors follow directory convention of base dir system/<nodeName>/<file.json>

E.g. diskUsage doesn't follow this at the moment.

host-collectors
├── diskUsage
│   └── gerard-kurl-0
│       └── diskUsage.json
└── system
    ├── gerard-kurl-0
    │   ├── block_devices.json
    │   ├── cpu.json
    │   ├── hostos_info.json
    │   └── memory.json
    └── node_list.json

Also, does this PR mean other analyzers will just have to implement method CheckCondition for the analyzing part?

[diamonwiggins]

Understand that all other analyzers will have to use function retrieveCollectedContents to retrieve the files before analyzing.

We will have to ensure that other host collectors follow directory convention of base dir system/<nodeName>/<file.json>

E.g. diskUsage doesn't follow this at the moment.

host-collectors
├── diskUsage
│   └── gerard-kurl-0
│       └── diskUsage.json
└── system
    ├── gerard-kurl-0
    │   ├── block_devices.json
    │   ├── cpu.json
    │   ├── hostos_info.json
    │   └── memory.json
    └── node_list.json

retreiveCollectedContents takes the base path and the file name as arguments, so in this case the base path would just be host-collectors/diskUsage. The only pattern everything must follow with this implementation is base-path/node/filename which is the pattern that remote host collection uses as well

Also, does this PR mean other analyzers will just have to implement method CheckCondition for the analyzing part?

Yup, every analyzer will just need to move their compare functions into a CheckCondition method, update Analyze with the simplified format, and add tests.

[banjoh]

I like the direction this PR has taken. This will DRY quite many of the host analysers. I've left some minor code comment changes

A change we can consider doing is moving node-list.json a directory higher to remove the duplicate file that gets created. It's not a major issue since it will contain the same contents.

[banjoh]

analyzeResult can be nil. Check if nil

[banjoh]

Make these structs private. They are not used outside of analyze package

[banjoh]

We will now have an analysis result for each node. I think we need to indicate which node a results is for. A prefix perhaps?

[banjoh]

We will now have an analysis result for each node. I think we need to indicate which node a results is for. A prefix perhaps?

Ignore this comment. I just saw code doing this

[nvanthao]

Hi @banjoh, could you help me review the updates? 🙇

[banjoh]

The function to build collectors and filter excluded ones was making wrapping tracing spans a problem. We need one otel.Trace(...).Start() per collector. With that function, we would either not capture excluded collectors, or end up having duplicate traces since we call Start() twice for collectors that get executed.

[banjoh]

LGTM