vtorc: add detected_problems counter

[yields]

Description

The metric allows operators to alert on different analysis problems that vtorc detects; this is important because some of the recoveries can be manually disabled and recovered, such as ERS. I suggest we add a detected_problems field with an analysis_code label that allows the operator to alert on different issues.

Related Issue(s)

#13966

Checklist

• "Backport to:" labels have been added if this change should be back-ported
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on the CI
• Documentation was added or is not required

Deployment Notes

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[deepthi]

This is a good first attempt. However, it is not usable / actionable unless the metric is keyed by keyspace/shard.
See #13723 for an example.
An increasing counter is also not useful for alerting on. What we need to know is whether a problem is currently active. So it should only have 0/1 values. Set it to 1 when the problem is detected, and 0 when it is resolved.
For some problem codes/types, this might mean that keyspace/shard dimensions are not sufficient, we may also need to add a tabletAlias dimension to the stat.

[deepthi]

The new iteration is much better. I still don't believe a counter is the right solution for alerting on problems though. The monitoring system would have to record the values and alert when they go up. It is much simpler with prometheus alert manager to alert on the current value of the metric versus having to store and compare.
@aquarapid what are your thoughts?

[aquarapid]

Typical PROM alerting for a production system would often alert on a rate-of-change basis on many types of metrics, as opposed to presence vs absence, so I don't see it as a problem to have a counter. It is up to the (human) operator creating the alerting to decide which problems they care about, and at which levels, even if some of these specific counters are in the category of "zero is good, everything else is bad". But for those you can just adjust your alerting expressions accordingly.

[GuptaManan100]

@aquarapid The problem with a counter in VTOrc perspective, is going to be that VTOrc would keep detecting the same problem over and over and it would continuously keep incrementing the counter until the problem is fixed. The actual value of the metric then would really be meaningless, it won't be the number of failures or anything, just the number of times VTOrc has detected the said failure so far. So, making it a binary metric makes more sense to me.

[GuptaManan100]

If we decide to make the metric a boolean gauge instead, then we would need to first clear the old values and set 1 for the analysis received.

[deepthi]

If we decide to make the metric a boolean gauge instead, then we would need to first clear the old values and set 1 for the analysis received.

Yes, let's do that. I chatted with @aquarapid and explained the problem with making it a counter. For the record, the problem is what Manan described - we'll be incrementing the counter every time vtorc checks for problems until it is fixed. So it could be 1000, or 10000 or a million and the exact number has no significance.

[GuptaManan100]

@yields Website updates are also required. The newly introduced metrics should be added to https://vitess.io/docs/18.0/reference/vtorc/ui_api_metrics/

[yields]

If we decide to make the metric a boolean gauge instead, then we would need to first clear the old values and set 1 for the analysis received.

Yes, let's do that. I chatted with @aquarapid and explained the problem with making it a counter.

If we go with a gauge I believe we'll need to preserve state, as CheckAndRecover receives problems (replicationAnalaysis) list and the AnalysisCode can be inst.NoProblem, so to set the gauge back to 0 we would need some kind of boolean on the replication analysis instead of a inst.NoProblem, is that right?

We could of course do this:

for _, code := range inst.AnalysisCodes() {
	gauge.Set(code, 0)
}

// intentionally iterating entries in random order
for _, j := range rand.Perm(len(replicationAnalysis)) {
	analysisEntry := replicationAnalysis[j]

	if code := analysisEntry.Analysis; code != inst.NoProblem {
		gauge.Set(code, 1)
	}

	go func() {
		err = executeCheckAndRecoverFunction(analysisEntry)
		if err != nil {
			log.Error(err)
		}
	}()
}

But with this we lose the labels, plus the gauge won't really be meaningful as it can be 0 while an issue is still "active"

[GuptaManan100]

@yields I looked at the implementation of CountersWithMultiLabels to see how we could circumvent this issue. I found out that it is essentially just using a counters internally and converting all the labels into a string with a function call like safeJoinLabels.

What I am thinking of doing is something like this -

        labelsAdded := map[string]bool{}
	for _, analysisEntry := range replicationAnalysis {
		label := detectedProblemsCounter.GetLabelName([]string{string(analysisEntry.Analysis),
			analysisEntry.AnalyzedInstanceAlias,
			analysisEntry.AnalyzedKeyspace,
			analysisEntry.AnalyzedShard})
		labelsAdded[label] = true
		detectedProblemsCounter.Set(label, 1)
	}
	for label, _ := range detectedProblemsCounter.Counts() {
		if ! labelsAdded[label] {
			// Remove this from the detectedProblemsCounter
			detectedProblemsCounter.Reset(label)
		}
	}

We would presumably need to add a couple of functions to the CountersWithMultiLabels, one of which being -

func (mc *CountersWithMultiLabels) GetLabelName(names []string) string {
	return safeJoinLabels(names, mc.combinedLabels)
}

But this would work as intended I believe. Essentially, we are setting the counters to 1 while keeping track of all the labels we set. In the end we remove whatever we didn't set. We won't have the problem of a label going to 0 even the problem still exists. But yes, you are right, I don't see a way of doing this without creating a map (albeit a local one).

[yields]

Personally I'm against using the counters that way (I don't see an issue with them at all, it's up to the operator to set alerts etc), but I pushed a commit that implements what you've suggested.

Also CountersWithMultiLabels doesn't have .Set(), so used .Reset() and then .Add().

[yields]

This looked like a bug to me so changed the scoping here.

[GuptaManan100]

Oh yes! Good catch.

[GuptaManan100]

@yields You're right counters are meant to be increasing, gauges are meant to go up and down. We should use GaugesWithMultiLabels instead of CountersWithMultiLabels for our purpose. We also have Set function available on it. Rest LGTM!

[yields]

@GuptaManan100 Awesome, see the latest commits :)

[yields]

I don't get why so many tests are failing. Maybe try merging main. Also website docs PR is required for these changes.

Rebased, I'll also create a PR for the docs website.

[GuptaManan100]

Also, it might be prudent to add e2e testing for the produced metrics too. We already have the framework to do this in the vtorc end-to-end test. For example, you can look at WaitForSuccessfulRecoveryCount in the go/test/endtoend/vtorc/utils/utils.go directory.

[GuptaManan100]

Just e2e testing and website docs are left, and then this is good to go!

[GuptaManan100]

@yields It would be great if you can make the required changes as soon as possible so that we can merge this PR before the v18 rc-1 which is due in 2 days.

[GuptaManan100]

Perfect!

[deepthi]

Thank you!