fix: return provider response during fnc listing errors #1763
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Return the original upstream response body when the the list request returns an error. In general, the provider is returning useful and actionable error messages for the user, the previous code hid this in the logs and this is easy for user to overlook. Additionally, remove an early return from error case after fetching metrics. This looked like a bug and could result in empty api responses if there was a prometheus error. Signed-off-by: Lucas Roesler <[email protected]>
alexellis
reviewed
Oct 24, 2022
| @@ -55,6 +56,34 @@ func Test_PrometheusMetrics_MixedInto_Services(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func Test_MetricHandler_ForwardsErrors(t *testing.T) { | |||
There was a problem hiding this comment.
Could you add one more test to show the positive case, if we have that already, just let me know and I'll go ahead and approve.
alexellis
reviewed
Oct 24, 2022
| @@ -34,15 +34,13 @@ func AddMetricsHandler(handler http.HandlerFunc, prometheusQuery PrometheusQuery | |||
| log.Printf("List functions responded with code %d, body: %s", | |||
| recorder.Code, | |||
| string(upstreamBody)) | |||
|
|
|||
| http.Error(w, "Metrics handler: unexpected status code from provider listing functions", http.StatusInternalServerError) | |||
| http.Error(w, string(upstreamBody), recorder.Code) | |||
There was a problem hiding this comment.
I think I am OK with this change, let's hope nobody is looking for that specific message because I think it's been there a long time.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Return the original upstream response body when the the list request returns an error. In general, the provider is returning useful and actionable error messages for the user, the previous code hid this in the logs and this is easy for user to overlook.
Additionally, remove an early return from error case after fetching metrics. This looked like a bug and could result in empty api responses if there was a prometheus error.
Motivation and Context
design/approvedlabelThis is related to Feat add startup error when running in kube system faas-netes#1031 and Unable to use kube-system for OpenFaaS functions faas-netes#1022
How Has This Been Tested?
Additional test has been added
Types of changes
Checklist:
git commit -s