Add namespace in function name for metrics #1488

viveksyngh · 2020-03-22T08:39:54Z

This commit adds namespace in function names while logging metrics to
prometheus, irrespective of the function is invoked with namespace suffix
or not.

This is also required to add multiple namespace support to faas-idler

https://github.com/openfaas-incubator/faas-idler/issues/37 which is part
of openfaas/faas-netes#511

Signed-off-by: Vivek Singh [email protected]

Description

Motivation and Context

I have raised an issue to propose this change (required)
My issue has received approval from the maintainers or lead with the design/approved label

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have signed-off my commits with git commit -s
I have added tests to cover my changes.
All new and existing tests passed.

gateway/handlers/notifiers.go

gateway/metrics/exporter.go

LucasRoesler · 2020-03-22T12:06:23Z

gateway/metrics/exporter.go

+					e.services = services
+				} else {
+					for _, namespace := range namespaces {
+						services, err := e.getFunctions(endpointURL, namespace)


I think this works well for now, but I also suspect a larger discussion is needed about how/if we move this into the provider sending a request per namespace every second seems like something the provider should encapsulate so that it can optimize that behavior as it best makes sense for itself.

This current implementation would create a "thundering herd"-like problem with no way to really streamline or optimize it. For example, the k8s provider should be able to request the existing functions from several/all namespaces in a single request.

Perhaps we need a new endpoint in the provider that allows the gateway to send 0, 1, or more namespaces as part of the request. When it is 0, it would request for all namespaces, otherwise the response would send back a list of functions for the provided namespaces. Alternatively, we could allow a special _all value. Then 0 namespaces == default, 1 or more applied as obvious, _all return functions from all possible namespaces.

I completely agree and we should move function aggregation logic to the provider.

I think we can modify the exiting function list endpoint using some special query parameters like _all, for which will combine functions from all namespaces and return it. Otherwise it works as it is working now. I think it will be a very minimal change compared to having a new endpoint. For new endpoint we will have to write different logic depending on the provider at several places.

Introducing a thundering herd issue concerns me. What are the alternatives to doing that?

Thundering herd issue is because every 5 second (configurable) it tries to get all namespaces and then for each namespace it lists its functions to count service/replica count of functions.

Solution is to use a new endpoint or existing which list all functions across all namespaces so that we don't have to make these multiple calls. This implementation can be done in faas-netes which will be efficient.

@viveksyngh can you write a bash loop to deploy 100-200 functions and then measure the effect of the patch?

for j in {1..10} do ; kubectl create namespace $i #annotate it for i in {1..10} do ; faas-cli deploy --image figlet --name fn$I --namespace $j done done

Perhaps create 10-20 in 10 different namespaces

gateway/metrics/exporter.go

alexellis · 2020-04-05T08:50:33Z

gateway/metrics/exporter.go

@@ -72,9 +82,45 @@ func (e *Exporter) StartServiceWatcher(endpointURL url.URL, metricsOptions Metri
 	ticker := time.NewTicker(interval)
 	quit := make(chan struct{})

-	timeout := 3 * time.Second
+	go func() {
+		for {


What is this new segment of code for? I was mainly expecting this PR to set a default suffix in the invocation URL when there was not one given.

New segment of code is for getting replica count of the services, which was not having support for multiple namespace. It was only getting replica count for the functions present in default namespace.

alexellis · 2020-07-07T07:44:00Z

This was reviewed several weeks ago in a members call with @viveksyngh and @LucasRoesler - we had a concern about performance and paused the patch. We need to review the solution and see if there are alternatives with other trade-offs (this still may be the best option)

viveksyngh · 2020-07-29T11:38:51Z

Testing :

k3d create 

export KUBECONFIG="$(k3d get-kubeconfig --name='k3s-default')"

arkade install openfaas --set clusterRole=true



kubectl port-forward -n openfaas svc/gateway 8080:8080 &

PASSWORD=$(kubectl get secret -n openfaas basic-auth -o jsonpath="{.data.basic-auth-password}" | base64 --decode; echo)

echo -n $PASSWORD | faas-cli login --username admin --password-stdin

kubectl -n openfaas run \
--image=stefanprodan/faas-grafana:4.6.3 \
--port=3000 \
grafana

kubectl -n openfaas expose deployment grafana \
--type=NodePort \
--name=grafana

kubectl port-forward -n openfaas svc/grafana 3000:3000 &

kubectl port-forward -n openfaas svc/prometheus 9090:9090 &

kubectl create namespace fn

kubectl annotate ns/fn openfaas=1

faas-cli namespaces
Namespaces:
 - fn
 - openfaas-fn



faas-cli store deploy figlet 

faas-cli store deploy figlet -n fn

faas-cli store deploy nodeinfo -n fn

faas-cli store deploy leftpad

echo "Hello" | faas-cli invoke figlet -n fn

echo "Hello" | faas-cli invoke figlet

echo "Hello" | faas-cli invoke leftpad

echo "Hello" | faas-cli invoke nodeinfo -n fn


➜  ~ faas-cli list -n fn
Function                      	Invocations    	Replicas
figlet                        	1              	1
nodeinfo                      	0              	1
➜  ~ faas-cli list
Function                      	Invocations    	Replicas
figlet                        	1              	1
leftpad                       	1              	1

Before Changes

After Changes

Edit the Gateway deployment to use docker image viveksyngh/gateway:metrics-with-namespace-1

faas-cli list -n openfaas-fn
Function                      	Invocations    	Replicas
figlet                        	2              	1
leftpad                       	1              	1
➜  ~ faas-cli list -n fn
Function                      	Invocations    	Replicas
nodeinfo                      	5              	1
figlet                        	3              	1

alexellis · 2020-09-16T16:14:33Z

gateway/metrics/exporter.go

+	return services, nil
+}
+
+func (e *Exporter) getNamespaces(endpointURL url.URL) ([]string, error) {


Can we use the CLI's code for this?

alexellis · 2020-09-16T16:14:54Z

gateway/metrics/exporter.go

-		for {
-			select {
-			case <-ticker.C:
+func (e *Exporter) getFunctions(endpointURL url.URL, namespace string) ([]types.FunctionStatus, error) {


Can you use the CLI here instead?

alexellis · 2020-10-29T10:07:25Z

@viveksyngh we will need a rebase on this.

After some discussion with @LucasRoesler, we thought this PR should be merged and then load-tested later.

This commit adds namespace in function names while logging metrics to prometheus, irrespective of the function is invoked with namespace suffix or not. This is also required to add multiple namespace support to faas-idler https://github.com/openfaas-incubator/faas-idler/issues/37 which is part of openfaas/faas-netes#511 Signed-off-by: Vivek Singh <[email protected]>

Signed-off-by: Vivek Singh <[email protected]>

alexellis · 2020-10-30T08:13:30Z

@LucasRoesler could you give this a final pass?

LucasRoesler

@alexellis I think we are ready. Let's follow up in a future PR to consolidate those places we can/should use the cli

alexellis

Approved

LucasRoesler reviewed Mar 22, 2020

View reviewed changes

alexellis reviewed Apr 5, 2020

View reviewed changes

alexellis mentioned this pull request Jul 21, 2020

Function invocation incorrect for multiple namespaces #1413

Closed

5 tasks

alexellis reviewed Sep 16, 2020

View reviewed changes

viveksyngh added 3 commits October 29, 2020 22:28

Use default namespace in metrics exporter

8224cfa

Signed-off-by: Vivek Singh <[email protected]>

Use namespace while filtering metric result

3118134

Signed-off-by: Vivek Singh <[email protected]>

viveksyngh force-pushed the metrics-with-namespace branch from f0e633a to 3118134 Compare October 29, 2020 16:58

LucasRoesler approved these changes Nov 1, 2020

View reviewed changes

alexellis approved these changes Nov 2, 2020

View reviewed changes

alexellis merged commit c9c2b39 into openfaas:master Nov 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add namespace in function name for metrics #1488

Add namespace in function name for metrics #1488

viveksyngh commented Mar 22, 2020 •

edited

Loading

LucasRoesler Mar 22, 2020

viveksyngh Mar 22, 2020

alexellis Apr 5, 2020

viveksyngh Apr 5, 2020

alexellis Sep 16, 2020

alexellis Sep 16, 2020 •

edited

Loading

alexellis Apr 5, 2020

viveksyngh Apr 5, 2020

alexellis commented Jul 7, 2020

viveksyngh commented Jul 29, 2020 •

edited

Loading

alexellis Sep 16, 2020

alexellis Sep 16, 2020

alexellis commented Oct 29, 2020

alexellis commented Oct 30, 2020

LucasRoesler left a comment

alexellis left a comment

Add namespace in function name for metrics #1488

Add namespace in function name for metrics #1488

Conversation

viveksyngh commented Mar 22, 2020 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis commented Jul 7, 2020

viveksyngh commented Jul 29, 2020 • edited Loading

Before Changes

After Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis commented Oct 29, 2020

alexellis commented Oct 30, 2020

LucasRoesler left a comment

Choose a reason for hiding this comment

alexellis left a comment

Choose a reason for hiding this comment

viveksyngh commented Mar 22, 2020 •

edited

Loading

alexellis Sep 16, 2020 •

edited

Loading

viveksyngh commented Jul 29, 2020 •

edited

Loading