Add stream upstream, stream server zones metrics support #7

Dean-Coakley · 2018-08-28T09:42:26Z

Added support for stream upstream metrics
Added support for stream server zone metrics
Added stream server zone to nginx.conf for testing purposes
Extended GetStats to include stream metrics
Added integration test to validate that stream metrics are correct
Fixed typo reponse-> response

* Added support for stream upstream metrics * Added support for stream server zone metrics * Added stream server zone to nginx.conf for testing purposes * Extended GetStats to include stream metrics * Added integration test to validate that stream metrics are returned

isaachawley

👍

pleshakov

Please see my suggestions.

pleshakov · 2018-08-28T11:15:37Z

tests/client_test.go

+		t.Fatalf("Error connecting to nginx: %v", err)
+	}
+
+	// need upstream for stats


-> need an upstream for stats

pleshakov · 2018-08-28T11:21:01Z

tests/client_test.go

+		t.Errorf("Error adding stream upstream server: %v", err)
+	}
+
+	// make request so we have stream server zone stats, expect 5xx error


since we're dealing with stream module here, to make things less confusing, try to establish a TCP connection using "net" package instead of using the httpclient. See https://golang.org/pkg/net/

Change the comment to something like below:

// Establish a connection to the stream server, so that we have some stats available for the stream zone.

Do not expect an error. If you want to have a simple stream server that responds something, see http://nginx.org/en/docs/stream/ngx_stream_return_module.html#return

pleshakov · 2018-08-28T11:22:43Z

tests/client_test.go

+	if stats.Connections.Active == 0 {
+		t.Errorf("Bad connections: %v", stats.Connections)
+	}
+	// SSL metrics blank in this example


what is this comment for?

pleshakov · 2018-08-28T11:32:13Z

tests/client_test.go

+			if upstream.Peers[0].Connections < 0 {
+				t.Errorf("stream upstream should have connects value")
+			}
+			if upstream.Peers[0].HealthChecks.LastPassed {


to test this, you must configure stream health checks http://nginx.org/en/docs/stream/ngx_stream_upstream_hc_module.html#health_check

test that the last health check has passed

client/nginx.go

pleshakov

please see my comments

pleshakov · 2018-08-30T11:40:16Z

Makefile

@@ -1,17 +1,24 @@
 NGINX_PLUS_VERSION=15-2
 NGINX_IMAGE=nginxplus:$(NGINX_PLUS_VERSION)

-test: docker-build run-nginx-plus test-run clean
-
+test: docker-build run-nginx-plus test-run config-no-stream test-no-stream clean


what if we remove the targets config-no-stream and test-no-stream and put all that config manipulation in the test-run target? I think it will make it less confusing

I like it this way, so that we can change the config and make test-run or make test-no-stream without changing the state of the container.

I also don't find this confusing.

client/nginx.go

pleshakov · 2018-08-30T12:01:38Z

client/nginx.go

 	if err != nil {
 		return fmt.Errorf("%v; failed to read the response body: %v", mainErr, err)
 	}

-	return fmt.Errorf("%v; error: %v", mainErr, apiErr.toString())
+	return &internalError{
+		err:      fmt.Sprintf("%v; error: %v", mainErr, apiErrResp.toString()),


I don't think we need to store the mainErr inside internalError, as the apiErr that we get from Plus describes the problem very well.

This is the same thing it did before see master.

pleshakov · 2018-08-30T12:05:04Z

tests/client_no_stream_test.go

+		t.Errorf("Error getting stats: %v", err)
+	}
+
+	if stats.Connections.Active == 0 {


why do we make this check?

It's a sanity check. We should get some connections from the api even without the stream block. I'll make it clearer.

- Improve test error clarity

- Add internalError.Wrap for preserving context

pleshakov · 2018-08-31T11:43:53Z

Makefile


 test-run:
-	go test client/*
-	go test tests/client_test.go
+	GOCACHE=off go test client/*


for unit tests in client, don't see why we need to GOCACHE=off

Without it, changes in templates won't get tested. Because the go code doesn't change, the test results will be cached.

for this ones https://github.com/nginxinc/nginx-plus-go-sdk/blob/master/client/nginx_test.go we don't need any template

ugh alright

pleshakov · 2018-08-31T11:46:44Z

client/nginx.go

-		return fmt.Errorf("%v; failed to read the response body: %v", mainErr, err)
+		return &internalError{
+			err:      fmt.Sprintf("failed to read the response body: %v", err),
+			apiError: apiError{},


apiError: apiError{} is it necessary?

pleshakov · 2018-08-31T11:47:58Z

client/nginx.go

-		return createResponseMismatchError(resp.Body, mainErr)
+		return createResponseMismatchError(resp.Body).Wrap(fmt.Sprintf(
+			"failed to complete delete request: expected %v response, got %v",
+			http.StatusCreated, resp.StatusCode))


http.StatusCreated -> http.StatusOK

pleshakov · 2018-08-31T11:48:17Z

client/nginx.go

-		return createResponseMismatchError(resp.Body, mainErr)
+		return createResponseMismatchError(resp.Body).Wrap(fmt.Sprintf(
+			"expected %v response, got %v",
+			http.StatusCreated, resp.StatusCode))


http.StatusCreated -> http.StatusOK

pleshakov · 2018-08-31T11:49:58Z

client/nginx.go

@@ -594,17 +679,29 @@ func (client *NginxClient) GetStats() (*Stats, error) {
 		return nil, fmt.Errorf("failed to get stats: %v", err)
 	}

+	streamZones, err := client.getStreamServerZones()


please move it after getUpstreams call

pleshakov · 2018-08-31T11:56:41Z

tests/client_test.go

+	}
+
+	server := client.StreamUpstreamServer{
+		Server: "127.0.0.1:8080",


we're using the virtual server for the API. perhaps it makes sense to have a separate server for testing defined in the stream block. Please take a look at http://nginx.org/en/docs/stream/ngx_stream_return_module.html#return

- Fix StatusOK vs StatusCreated copy/paste typo - Re-order stats calls - Implicit empty apiError

Dean-Coakley added 2 commits August 27, 2018 16:37

Fix response typo. reponse->response

0c34234

Dean-Coakley added the enhancement Pull requests for new features/feature enhancements label Aug 28, 2018

Dean-Coakley self-assigned this Aug 28, 2018

Dean-Coakley requested review from isaachawley and pleshakov August 28, 2018 09:42

Fix Makefile docker port mapping. Fix comments

2b059ca

isaachawley approved these changes Aug 28, 2018

View reviewed changes

pleshakov requested changes Aug 28, 2018

View reviewed changes

isaachawley assigned isaachawley and unassigned Dean-Coakley Aug 29, 2018

isaac added 2 commits August 29, 2018 12:43

Handle missing stream block

6adc4f1

Add stream healthcheck test

06d8dbe

pleshakov requested changes Aug 30, 2018

View reviewed changes

isaac added 2 commits August 31, 2018 09:39

Add sleep to wait for health checks to pass

2745a45

- Improve test error clarity

Simplify internal error handling

1ae242a

- Add internalError.Wrap for preserving context

pleshakov requested changes Aug 31, 2018

View reviewed changes

isaac added 2 commits August 31, 2018 13:21

Fix copy/paste typos

fbe16c5

- Fix StatusOK vs StatusCreated copy/paste typo - Re-order stats calls - Implicit empty apiError

Cache client unit tests

94ad6a0

pleshakov approved these changes Aug 31, 2018

View reviewed changes

isaachawley merged commit c9fb4ac into master Sep 4, 2018

isaachawley deleted the stream-metrics-support branch September 4, 2018 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stream upstream, stream server zones metrics support #7

Add stream upstream, stream server zones metrics support #7

Dean-Coakley commented Aug 28, 2018

isaachawley left a comment

pleshakov left a comment

pleshakov Aug 28, 2018

pleshakov Aug 28, 2018

pleshakov Aug 28, 2018

pleshakov Aug 28, 2018

pleshakov Aug 28, 2018

pleshakov left a comment

pleshakov Aug 30, 2018

isaachawley Aug 31, 2018

pleshakov Aug 30, 2018

isaachawley Aug 31, 2018

pleshakov Aug 30, 2018

isaachawley Aug 31, 2018

pleshakov Aug 31, 2018

isaachawley Aug 31, 2018

pleshakov Aug 31, 2018

isaachawley Aug 31, 2018

pleshakov Aug 31, 2018

pleshakov Aug 31, 2018

pleshakov Aug 31, 2018

pleshakov Aug 31, 2018

pleshakov Aug 31, 2018

Add stream upstream, stream server zones metrics support #7

Add stream upstream, stream server zones metrics support #7

Conversation

Dean-Coakley commented Aug 28, 2018

isaachawley left a comment

Choose a reason for hiding this comment

pleshakov left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleshakov left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment