Never use custom metrics interface for signalfx metrics adapter, PAASTA-17091 #2979
Conversation
…akes Paasta always generate HPA configurations that use the external metrics interface to access the Signalfx metrics adapter. PAASTA-17091. Known issue: this produces bogus signalflow code for horizontal_autoscaling uwsgi -- the code generated will query all uwsgi time series, without filtering.
| if ( | ||
| autoscaling_params.get("forecast_policy") == "moving_average" | ||
| or "offset" in autoscaling_params | ||
| or load_system_paasta_config().get_hpa_always_uses_external_for_signalfx() | ||
| ): | ||
| hpa_metric_name = self.namespace_external_metric_name(metrics_provider) | ||
| signalflow = LEGACY_AUTOSCALING_SIGNALFLOW.format( |
There was a problem hiding this comment.
I think this means that all instances using the metrics_provider: uwsgi or http will end up with an implicit forecast_policy: moving_average -- is that what we want?
There was a problem hiding this comment.
(oops meant to start a review -- not done!)
There was a problem hiding this comment.
(There probably aren't too many instances using uwsgi or http without moving_average I bet)
There was a problem hiding this comment.
I think this looks roughly good but I want to confirm that we're ok with forcing everything to use moving_average (my guess is that's ok?)
I also didn't look super closely at the removed itest yelpsoaconfigs -- it doesn't really look like we're losing any coverage, but can you confirm that's the case?
| "horizontal_autoscaling": { | ||
| "type": "object", | ||
| "additionalProperties": false, | ||
| "properties": { |
There was a problem hiding this comment.
It looks like there's one instance of compute-infra-test-service in norcal-stagef still using this section, so you probably need to change/remove it before shipping this
| if ( | ||
| autoscaling_params.get("forecast_policy") == "moving_average" | ||
| or "offset" in autoscaling_params | ||
| or load_system_paasta_config().get_hpa_always_uses_external_for_signalfx() | ||
| ): | ||
| hpa_metric_name = self.namespace_external_metric_name(metrics_provider) | ||
| signalflow = LEGACY_AUTOSCALING_SIGNALFLOW.format( |
There was a problem hiding this comment.
(There probably aren't too many instances using uwsgi or http without moving_average I bet)
* Add autoscaling configuration validation Let's ensure that people don't try to set a (setpoint, offset) pair that is invalid. NOTE: I've deleted the existing autoscaling validation code as it was only checking for an autoscaling configuration that we moved away from (`horizontal_autoscaling`) - see #2582 for the initial implementation of this and #2979 for its removal.
The first commit, ec85007, makes it so we don't ever use the custom metrics interface for the signalfx metrics adapter, only the external interface.
The second commit, e76906f, removes all code dealing with
horizontal_autoscaling, since nothing uses this, and the first commit likely introduces a bug in this code w.r.t. uwsgi autoscaling. (It would generate signalflow code that would query all uwsgi timeseries with no filtering.)