Add failureThreshold to elastic-agent self-monitoring config

[pchila]

What does this PR do?

Use failure_threshold introduced in elastic/beats#41570 in self-monitoring configuration to avoid elastic-agent reporting DEGRADED if it fails to fetch metrics due to a component starting/stopping.
The default value for the failure threshold is set to 2 but it can be configured via config file or fleet policy.

Why is it important?

It is important to avoid a misrepresentation of agent status due to a single metrics fetch erroring out once.
See #5332

Checklist

• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• [ ] I have made corresponding changes to the documentation
• [ ] I have made corresponding change to the default configuration files
• I have added tests that prove my fix is effective or that my feature works
• [ ] I have added an entry in ./changelog/fragments using the changelog tool
• [ ] I have added an integration test or an E2E test

Disruptive User Impact

How to test this PR locally

Related issues

• Requires Metricbeat: add configurable failure threshold before reporting streams as degraded beats#41570
• Closes Agent gets unhealthy temporarily because Beat monitoring sockets are not available #5332

Questions to ask yourself

• How are we going to support this in production?
• How are we going to measure its adoption?
• How are we going to debug this?
• What are the metrics I should take care of?
• ...

[elasticmachine]

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

[elasticmachine]

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

[mergify]

This pull request does not have a backport label. Could you fix it @pchila? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-./d./d is the label to automatically backport to the 8./d branch. /d is the digit

[mergify]

backport-v8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

[blakerouse]

Looks good to me.

[elastic-sonarqube]

Quality Gate passed

Issues
2 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
80.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

[michalpristas]

based on this configuration default value for FailureThreshold is nil
but it is not true as we set default value if it's not set.
shouldn't we preset it here in DefaultConfig() to make expectations clear?
i know you took metrics period as an example. not a blocker just thinking out loud

[michalpristas]

also why *uint seems unconventional and it does not match beat side implementation either.

[pchila]

The reason why the defaults here don't match the beats default is for backward compatibility: until now the first error on a stream in beats would trigger a status change to DEGRADED and beats maintains that behavior by default.

Here on the agent side the reason for *uint comes from:

• zero value (0) would disable any status change for the monitoring metricbeat streams
• the default is managed in internal/pkg/agent/application/monitoring/v1_monitor.go when we are sure there's been no set of value in config (pointer == nil)

I suppose I could set the default value in func DefaultConfig() *MonitoringConfig but if by any chance the Monitoring config is deserialized without calling DefaultConfig() we would get an erraneous value set in the failureThreshold (0).
Another case I am concerned with is unmarshaling from a fleet policy that does not specify any value for the threshold: if we set 2 or 0 by default there we could be overriding some other value...
Overall having a clear "not set" value that matches the go zero value seemed a safer approach

[michalpristas]

can you elaborate about backward compatibility?
in a matrix of config/agent
old/old and new/new we don't care
old/new - means we have old config and we should aim for safer behavior
new/old - new config with old agent we don't understand newly introduced keywords and ignore them, behavior is not there

I suppose I could set the default value in func DefaultConfig() *MonitoringConfig but if by any chance the Monitoring config is deserialized without calling DefaultConfig() we would get an erraneous value set in the failureThreshold (0).

this applies for probably any config we have

[pchila]

can you elaborate about backward compatibility? in a matrix of config/agent old/old and new/new we don't care old/new - means we have old config and we should aim for safer behavior new/old - new config with old agent we don't understand newly introduced keywords and ignore them, behavior is not there

When I mentioned backward compatibility I was referring to metricbeat and the reason why the defaults in agent don't match the one in beats as it was mentioned in an earlier comment (quoted below)

also why *uint seems unconventional and it does not match beat side implementation either.

What I meant with "backward compatibility" was not "backward compatibility between agent and beats version" but rather "metricbeat default value for failureThreshold takes into account the current behaviour when a metricset stream errors out during fetch"

There's no need for a config/agent old/new matrix as we want to change the default behaviour of monitoring config in order to solve #5332.
The possibility to configure the value in the monitoring config has been added as a way to change/disable the DEGRADED status if we need to mitigate an issue or change behavior testing purposes (it's an escape hatch of sorts).

I suppose I could set the default value in func DefaultConfig() *MonitoringConfig but if by any chance the Monitoring config is deserialized without calling DefaultConfig() we would get an erraneous value set in the failureThreshold (0).

this applies for probably any config we have

In this case, all values of uint are valid configuration values (there's no not set value).
The fact that we have already other configuration values that cannot represent "not explicitly set" because of default values, should not preclude to define a new config value as *uint so that a nil value to be interpreted as "value not set".
I am not sure that following what has already been done for other configurations would have definite advantages in this case.