DataDog provider should allow dealing with no data points returned

[jessesuen]

Summary

When there are no data points in a DataDog query, we automatically return error: Datadog returned no value with no ability to change this behavior. However, some users may want to consider this scenario a success. The code which prevents users from deciding how to handle this is here:

argo-rollouts/metricproviders/datadog/datadog.go

Lines 142 to 150 in f5e2d82

	if len(res.Series) < 1 || len(res.Series[0].Pointlist) < 1 {
	return "", v1alpha1.AnalysisPhaseError, fmt.Errorf("Datadog returned no value: %s", string(bodyBytes))
	}
	series := res.Series[0]
	datapoint := series.Pointlist[len(series.Pointlist)-1]
	if len(datapoint) < 1 {
	return "", v1alpha1.AnalysisPhaseError, fmt.Errorf("Datadog returned no value: %s", string(bodyBytes))
	}

The above logic does not allow a query to return with no datapoints because we error if the array length is 0.

One possible solution, is to modify this code and allow the result to return as nil without marking the measurement as error. Then, users could decide how to deal with this by writing an expression like:

successCondition: result == nil || result <= 0.05

A caveat is that I forget if expr library allows for polymorphic values where result may be a float or nil and compared using built-in operators. But if it doesn't, we could always add a convenience function, IsNil() to allow users to deal with it.

successCondition: IsNil(result) || result <= 0.05

Diagnostics

What version of Argo Rollouts are you running? v1.1.0-rc1

---

Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritize the issues with the most 👍.

[jessesuen]

More context: https://cloud-native.slack.com/archives/C01U781DW2E/p1632782321129900

[rms1000watt]

@jessesuen Thank you for making this issue. I'll take a look at it.

[jessesuen]

I think to fix this:

1. Instead of doing this:

return "", v1alpha1.AnalysisPhaseError, fmt.Errorf("Datadog returned no value: %s", string(bodyBytes))

we instead call EvaluateResult the result and give value of nil. e.g.:

evaluate.EvaluateResult(nil, metric, p.logCtx)

2. If expr does not allow polymorphic comparison using built-in operators (i.e. successCondition: result == nil || result <= 0.05), then we could introduce an isNil() convenience method.

[rms1000watt]

I think the first option is not feasible.

There's 2 cases:
- datadog returned no values, and people want that to error out
- datadog returned no values, and people don't want that to error out, so they add a success/failure condition

If we force evaluate.EvaluateResult(nil, metric, p.logCtx) on every empty result, we'd have effectively broken backwards compatibility since it doesn't error out any more.

I mean, maybe that's OK? But I at least want to talk through that situation.

---

Yeah, this has to break backwards compatibility for the solution to work one way or another.. still thinking it through. Trying not to change the plumbing in the datadog provider, but rather provide extra hooks for the Developer when writing success/failure conditions.

---

If I were to follow the first option, it would break. I cannot pass in nil since it's untyped. If I were to assign a type and have a nil pointer, then the success/failure condition would break because of the type mismatch *float64 and float64

I'm adding more logic to preprocess if pointers are provided to the evaluation

[rms1000watt]

@jessesuen Ready for review: #1551 👍 🙏