Modify the Stat API to handle requests for resource type "all" #928

rmars · 2018-05-10T00:58:47Z

Allow the Stat endpoint in the public-api to accept requests for resourceType "all".
Currently, this queries Pods, Deployments, RCs and Services, but can be modified to query other things as well.

Both the CLI and web endpoints now work if you set resourceType to all.

Examples:

http://localhost:8084/api/stat?resource_type=all&namespace=emojivoto

$ go run cli/main.go stat all -n emojivoto --api-addr localhost:8085
NAME                           MESHED   SUCCESS      RPS   LATENCY_P50   LATENCY_P95   LATENCY_P99
po/emoji-5f6854bc96-kmwqv         1/1   100.00%   0.3rps           5ms          10ms          10ms
po/emoji-5f6854bc96-r842n         1/1   100.00%   0.5rps           5ms          10ms          10ms
po/vote-bot-6659cc85cb-dldds      1/1         -        -             -             -             -
po/voting-844b495b9d-jj2vl        1/1         -        -             -             -             -
po/voting-844b495b9d-tplzn        1/1         -        -             -             -             -
po/web-77fdc7bc7d-k5lbt           1/1   100.00%   0.1rps           7ms          28ms          30ms
po/web-77fdc7bc7d-t9d7f           1/1         -        -             -             -             -

NAME                           MESHED   SUCCESS   RPS   LATENCY_P50   LATENCY_P95   LATENCY_P99
svc/emoji-svc                     2/2         -     -             -             -             -
svc/voting-svc                    2/2         -     -             -             -             -
svc/web-svc                       2/2         -     -             -             -             -

NAME                           MESHED   SUCCESS      RPS   LATENCY_P50   LATENCY_P95   LATENCY_P99
deploy/emoji                      2/2   100.00%   0.9rps           5ms          10ms          10ms
deploy/vote-bot                   1/1         -        -             -             -             -
deploy/voting                     2/2         -        -             -             -             -
deploy/web                        2/2   100.00%   0.1rps           7ms          28ms          30ms

siggy

so cool!

can you update the stat command help output near Valid resource types include:?

siggy · 2018-05-10T01:41:05Z

controller/api/public/stat_summary.go

@@ -56,9 +64,60 @@ func (s *grpcServer) StatSummary(ctx context.Context, req *pb.StatSummaryRequest
 		return nil, status.Errorf(codes.InvalidArgument, "service only supported as a target on 'from' queries, or as a destination on 'to' queries.")
 	}

+	statTables := make([]*pb.StatTable, 0)
+
+	if req.Selector.Resource.Type == k8s.All {


is it possible to add a test for this case in stat_summary_test.go?

siggy · 2018-05-10T01:45:59Z

cli/cmd/stat.go

+	nameHeader         = "NAME"
+	maxNameLength      = len(nameHeader)
+	namespaceHeader    = "NAMESPACE"
+	maxNamespaceLength = len(namespaceHeader)


i realize this makes for more code changes, but consider defining maxNameLength and maxNamespaceLength in writeStatsToBuffer, and then passing them into printStatTable, rather than sharing global vars between the functions.

siggy · 2018-05-10T01:56:10Z

cli/cmd/stat.go

@@ -185,6 +194,14 @@ func writeStatsToBuffer(resp *pb.StatSummaryResponse, w *tabwriter.Writer) {
 		}
 	}

+	fmt.Fprint(w, "\n\n")
+	for resourceType, stats := range statTables {
+		fmt.Fprintf(w, "\n%s\n\n", resourceType)


i'd like to keep the conduit cli as similar to kubectl as possible. that pattern appends the resource types onto the names when the all flag is used:

$ kubectl -n emojivoto get all NAME DESIRED CURRENT UP-TO-DATE AVAILABLE AGE deploy/emoji 1 1 1 1 50s deploy/vote-bot 1 1 1 1 48s deploy/voting 1 1 1 1 49s deploy/web 1 1 1 1 49s NAME DESIRED CURRENT READY AGE rs/emoji-8bfdd4cfb 1 1 1 50s ...

this has the added benefit that those strings can be cut/pasted into subsequent commands.

this all implies a bigger change, as the output of printStatTable needs to vary depending on whether the all flag is set, but i think that's for the best, as a single resource stat output carries some redundant info in this implementation:

$ bin/conduit -n emojivoto stat deploy deployments NAME MESHED SUCCESS RPS LATENCY_P50 LATENCY_P95 LATENCY_P99 emoji 1/1 100.00% 1.9rps 5ms 10ms 10ms vote-bot 1/1 - - - - - voting 1/1 79.66% 1.0rps 5ms 10ms 10ms web 1/1 90.68% 2.0rps 6ms 40ms 48ms

you'll need to leverage the shortest version of our resource strings somehow, currently utilized in CanonicalKubernetesNameFromFriendlyName.

siggy · 2018-05-10T02:04:55Z

controller/api/public/stat_summary.go

+		resultChan := make(chan resourceResult)
+
+		for _, resource := range resourceTypes {
+			req := &pb.StatSummaryRequest{


mind naming req something else, to not collide with the param passed in?

ah yeah, that would be less confusing

siggy · 2018-05-10T02:11:19Z

controller/api/public/stat_summary.go

+	statTables := make([]*pb.StatTable, 0)
+
+	if req.Selector.Resource.Type == k8s.All {
+		// request stats for all resourceTypes, in parallel


this is awesome! i think it will be a bit more readable if you remove the special casing for the single stat query, and instead do something like:

resources := []string{} if req.Selector.Resource.Type == k8s.All { resources = resourceTypes } else { resources = []string{req.Selector.Resource.Type} } // request stats for all resourceTypes, in parallel resultChan := make(chan resourceResult) for _, resource := range resources { req := &pb.StatSummaryRequest{ Selector: &pb.ResourceSelection{ Resource: &pb.Resource{ Type: resource, Namespace: req.Selector.Resource.Namespace, }, LabelSelector: req.Selector.LabelSelector, }, TimeWindow: req.TimeWindow, } go func() { resultChan <- s.resourceQuery(ctx, req) }() } statTables := make([]*pb.StatTable, 0) for i := 0; i < len(resources); i++ { result := <-resultChan if result.err != nil { return nil, util.GRPCError(result.err) } statTables = append(statTables, result.res) }

yeah! this is way more readable!

siggy · 2018-05-10T02:23:32Z

controller/api/util/api_utils.go

+	resourceType := p.ResourceType
+	var err error
+	if resourceType != k8s.All {
+		resourceType, err = k8s.CanonicalKubernetesNameFromFriendlyName(p.ResourceType)


what do you think about adding support for all into CanonicalKubernetesNameFromFriendlyName, to avoid the special casing here?

this would motivate a further change in (s *grpcServer) StatSummary, where we'd have to check that the FromResource or ToResource are not all, but maybe that's not a big deal, because we're already checking that the FromResource is not a svc, so it's kind of a similar check?

yeah, that's probably a good idea! then we can avoid checking everywhere else.

siggy

thanks for all the changes, the output is looking great!

i made a few more comments, i think you're also going to be a little conflict-y with #935 fyi.

siggy · 2018-05-11T00:34:39Z

cli/cmd/stat.go

+	}
+}
+
+func printStatTable(stats map[string]*row, resourceType string, w *tabwriter.Writer, maxNameLength int, maxNamespaceLength int) {
 	if len(stats) == 0 {


i just realized we need to move this len(stats) check up into writeStatsToBuffer, as that's where we should be checking for the presence of any stats, otherwise we panic:

$ bin/conduit stat deploy -n foo panic: runtime error: slice bounds out of range goroutine 1 [running]: github.com/runconduit/conduit/cli/cmd.renderStats(0xc42027db50, 0x1d6ea6d, 0xb, 0xc42023fd70, 0x0) /go/src/github.com/runconduit/conduit/cli/cmd/stat.go:129 +0x20a github.com/runconduit/conduit/cli/cmd.requestStatsFromAPI(0x1e84ec0, 0xc42027db30, 0xc42023fd70, 0xc4202ee630, 0x1, 0x3, 0x7ffeefbfeae6) /go/src/github.com/runconduit/conduit/cli/cmd/stat.go:119 +0x124

we can move the check like this:

diff --git a/cli/cmd/stat.go b/cli/cmd/stat.go index 79c7566..b576d9e 100644 --- a/cli/cmd/stat.go +++ b/cli/cmd/stat.go @@ -199,8 +199,12 @@ func writeStatsToBuffer(resp *pb.StatSummaryResponse, reqResourceType string, w } } - for resourceType, stats := range statTables { + if len(statTables) == 0 { + fmt.Fprintln(os.Stderr, "No traffic found.") + os.Exit(0) + } + for resourceType, stats := range statTables { if reqResourceType == k8s.All { printStatTable(stats, resourceType, w, maxNameLength, maxNamespaceLength) } else { @@ -211,11 +215,6 @@ func writeStatsToBuffer(resp *pb.StatSummaryResponse, reqResourceType string, w } func printStatTable(stats map[string]*row, resourceType string, w *tabwriter.Writer, maxNameLength int, maxNamespaceLength int) { - if len(stats) == 0 { - fmt.Fprintln(os.Stderr, "No traffic found.") - os.Exit(0) - } - headers := make([]string, 0) if allNamespaces { headers = append(headers,

ahhh, good catch

siggy · 2018-05-11T00:40:18Z

cli/cmd/stat.go

+		} else {
+			printStatTable(stats, "", w, maxNameLength, maxNamespaceLength)
+		}
+		fmt.Fprint(w, "\n")


nit: we can avoid the trailing \n with:

firstStat := true for resourceType, stats := range statTables { if !firstStat { fmt.Fprint(w, "\n") } firstStat = false if reqResourceType == k8s.All { printStatTable(stats, resourceType, w, maxNameLength, maxNamespaceLength) } else { printStatTable(stats, "", w, maxNameLength, maxNamespaceLength) } }

…types

siggy

Awesome!

TIOLI before merging, consider mentioning the new all flag in the Valid resource types include: section of stat --help.

rmars · 2018-05-11T19:46:22Z

OMG I intended to and I completely forgot - thanks!

* Fix bug where we were dropping parts of the StatSummaryRequest * Add tests for prometheus query strings and for failed cases Problem In #928 I rewrote the stat api to handle 'all' as a resource type. To query for all resource types, we would copy the Resource, LabelSelector and TimeWindow of the original request, and then go through all the resource types and set Resource.Type for each resource we wanted to get. The bug is that while we copy over some fields of the original request, we didn't copy over all of them - namely Resource.Name and the Outbound resource. So the Stat endpoint would ignore any --to or --from flags, and would ignore requests for a specific named resource. Solution Copy over all fields from the request. I've also added tests for this case. In this process I've refactored the stat_summary_test code to make it a bit easier to read/use.

rmars added area/controller area/telemetry labels May 10, 2018

rmars self-assigned this May 10, 2018

rmars requested a review from siggy May 10, 2018 00:58

siggy reviewed May 10, 2018

View reviewed changes

rmars added area/cli and removed area/telemetry labels May 11, 2018

siggy reviewed May 11, 2018

View reviewed changes

rmars added 12 commits May 11, 2018 10:30

Refactor resource query into its own function

bce79eb

Handle requests for all resources

0af4c58

Modify CLI to also query for all

fb36314

Modify CLI stat display to display stat tables for multiple resource …

9caff27

…types

Add All to k8s.CanonicalKubernetesNameFromFriendlyName

6714a9a

Remove special casing for single resource type

4c04863

Make output of conduit stat all more like kubectl

488076c

move maxNameLength and maxNamespace length to not be global variables

6d57f00

Add a test for StatSummary with resourceType all

bd6b2a7

Add Service to test for all resources

b135d38

Move len(stat) check, fix trailing newline

9a7ad0c

Merge master, resolve conflicts

2dd7938

rmars force-pushed the rmars/stat-all branch from 082a95a to 2dd7938 Compare May 11, 2018 18:10

siggy approved these changes May 11, 2018

View reviewed changes

Add 'all' to CLI help text

4eca3cc

rmars mentioned this pull request May 11, 2018

Modify the Namespaces page to use one call to the stat API #941

Closed

whitespace cleanups

5a842b1

rmars merged commit b8dc83f into master May 11, 2018

rmars deleted the rmars/stat-all branch May 11, 2018 21:35

This was referenced May 17, 2018

Consolidate calls to /metrics to use the new resource type all #963

Merged

Fix bug in the public-api where conduit stat params were ignored #971

Merged

olix0r mentioned this pull request Jun 26, 2018

Fully-implemented, resource-aware conduit stat #627

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify the Stat API to handle requests for resource type "all" #928

Modify the Stat API to handle requests for resource type "all" #928

rmars commented May 10, 2018 •

edited

Loading

siggy left a comment

siggy May 10, 2018

siggy May 10, 2018

siggy May 10, 2018

siggy May 10, 2018

rmars May 10, 2018

siggy May 10, 2018

rmars May 10, 2018

siggy May 10, 2018

rmars May 10, 2018

siggy left a comment

siggy May 11, 2018

rmars May 11, 2018

siggy May 11, 2018

siggy left a comment

rmars commented May 11, 2018

Modify the Stat API to handle requests for resource type "all" #928

Modify the Stat API to handle requests for resource type "all" #928

Conversation

rmars commented May 10, 2018 • edited Loading

siggy 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

siggy 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

siggy left a comment

Choose a reason for hiding this comment

rmars commented May 11, 2018

rmars commented May 10, 2018 •

edited

Loading