New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Silence errors for namespace listing #725
Conversation
Fixes: #724 Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
411aefa
to
8db91a6
Compare
namespaces := []string{} | ||
if clusterRole { | ||
namespaces = ListNamespaces(defaultNamespace, clientset) | ||
} else { | ||
namespaces = append(namespaces, defaultNamespace) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could slightly simplify this using
namespaces := []string{} | |
if clusterRole { | |
namespaces = ListNamespaces(defaultNamespace, clientset) | |
} else { | |
namespaces = append(namespaces, defaultNamespace) | |
} | |
namespaces := []string{defaultNamespace} | |
if clusterRole { | |
namespaces = ListNamespaces(defaultNamespace, clientset) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very open to a PR.
return func(w http.ResponseWriter, r *http.Request) { | ||
log.Println("Query namespaces") | ||
// log.Println("List namespaces") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just delete it
// log.Println("List namespaces") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I am still undecided about whether we should delete all happy-path logging and focus on the Prometheus metrics gathered from the gateway.
@@ -102,6 +103,7 @@ func MakeReplicaReader(defaultNamespace string, clientset *kubernetes.Clientset) | |||
lookupNamespace = namespace | |||
} | |||
|
|||
s := time.Now() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be more valuable as an actual prometheus metric. Having it in the logs is nice, but it means a person is going to make judgments on a few values as opposed to being able to collect and aggregate real data into graphs etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I think it is in there already 👍 This is another example of a log message we should probably delete.
Fixes: #724
Signed-off-by: Alex Ellis (OpenFaaS Ltd) alexellis2@gmail.com
Description
Silence errors for namespace listing
Motivation and Context
Fixes: #724
How Has This Been Tested?
To be tested through CI
Types of changes