Basic support for historical & realtime stats by pteichman · Pull Request #66 · fastly/cli

pteichman · 2020-05-08T21:35:29Z

This gives the Fastly CLI a foothold in the stats world. Output is very basic, but roughly matches the old Python CLI.

This requires as-yet unmerged changes to go-fastly, so it's not expected to pass the build test.

phamann

Looking good. Comments are mainly around project conventions and awareness of some of the helper packages to make your life easier :)

phamann · 2020-05-11T15:37:12Z

+		hitRate = float64((agg.Hits - agg.Miss - agg.Errors)) / float64(agg.Hits)
+	}
+
+	// TODO: parse the JSON more strictly so this doesn't need to be dynamic.


Noting the TODO here, do you want to do this as part of this PR or at a later date?

Let's do this later. I need to think about whether we can unify these APIs at all.

pteichman · 2020-05-11T21:12:47Z

Force pushed because I rebased the original commits; there are no changes to those aside from conflict fixes. Everything new is in the four new commits.

Edit: and the build is still broken while we wait to roll in a new go-fastly.

phamann

Not a major blocker, but It would be good to see some test coverage for these commands. Have a look at some of the other command tests for examples. Some thoughts:

You can parameterize the realtime client in app.Run() which will allow us to inject a mock client to control output, like we do for the main API client.
We favour integration style tests for the commands, i.e given these inputs I want this rendered output and have a collection of test helpers to help with this pattern.

👍 Other than the above - LGTM

pteichman · 2020-05-12T21:51:36Z

Meeting you halfway: this adds tests for the regions and historical commands, but leaves out realtime for now since it's an infinite loop and I want to put in the time to test it properly.

Force pushed to rebase on master: all of the new code is in the last commit.
And as before, this continues to fail its build until we land the go-fastly upgrade.

From here my plan is to minimize churn:

Get go-fastly up to date.
Rebase this thing on master & verify tests.
Merge.

mccurdyc · 2020-05-13T15:12:37Z

@pteichman go-fastly should be up to date now :)

#71

pteichman · 2020-05-13T18:31:03Z

(another rebase; resolving conflicts with the new trunk)

phamann · 2020-05-13T19:50:00Z

+// Exec implements the command interface.
+func (c *HistoricalCommand) Exec(in io.Reader, out io.Writer) error {
+	service, source := c.manifest.ServiceID()
+	if source != manifest.SourceUndefined {


This would still allow for an empty service ID to be sent in the request. Therefore we should error if none passed and always set the input field.

Suggested change

if source != manifest.SourceUndefined {

if source == manifest.SourceUndefined {

return errors.ErrNoServiceID

}

c.Input.Service = service

That's intentional, an empty service is a valid request: it will dump all of the services that api token can read.

👍 Makes sense, however I now feel uncomfortable that the "dump all services" behaviour is essentially an undocumented side-effect and not a first-class discoverable feature - which it should be.

Also the possibility of source being undefined is reduced for C@E services as the service-id is always persisted and read from the fastly.toml, which is why we have this pattern as it allows you to omit the --service-id flag within your project directory.

So a couple of ideas for the future, but don't need to block this PR being merged:

We add an additional --all-services (or similar) flag, which makes it a first-class feature and allows people to intentionally invoke the behaviour. (my preferred option)

We update the flag description for --service-id to document that if left blank will return all services.

This gives a lot more context on what's going wrong with output. cmp is already in the tree, so this isn't a new dependency. Failing test output now looks like: --- FAIL: TestApplication/help_argument_only (0.00s) run_test.go:75: output mismatch (-want +got): []string{ ... // 579 identical elements "-n, --name=NAME The name of the S3 logging object", "", - "stats regions", - "List stats regions", - "", - "", "stats historical [<flags>]", "Query historical stats", ... // 13 identical elements }

* Parse stats command flags directly into input structs * Improve some message strings

phamann

👍 LGTM -

Just had a play with it locally and really pleased with out this has taken shape!

phamann suggested changes May 11, 2020

View reviewed changes

phamann added the enhancement New feature or request label May 11, 2020

phamann changed the title ~~Basic fastly cli support for historical & realtime stats~~ Basic support for historical & realtime stats May 11, 2020

pteichman force-pushed the pteichman/stats branch from 1635662 to 428e00f Compare May 11, 2020 21:11

phamann approved these changes May 12, 2020

View reviewed changes

Comment thread pkg/app/run.go

pteichman force-pushed the pteichman/stats branch from 428e00f to ff80545 Compare May 12, 2020 21:48

pteichman force-pushed the pteichman/stats branch 3 times, most recently from afa8637 to ef19aaa Compare May 13, 2020 18:30

phamann reviewed May 13, 2020

View reviewed changes

pteichman added 12 commits May 18, 2020 10:18

Initial add of "stats" command tree and "stats regions"

8653adf

Initial add of "stats historical"

0b3e481

Initial add of "stats realtime"

f6ce790

Rename stats source files to match convention

114fef1

Use EnumVar for stats flag checking, rather than checkArgs

3a8c015

Incorporate review feedback on stats command parsers

e0b31e8

* Parse stats command flags directly into input structs * Improve some message strings

Switch to our cmp.Diff helper for run_test output checks

47b8590

Add tests for "stats regions" and "stats historical"

7ba02fc

Resolve lint errors

ba1c45e

Remove an unused manifest in HistoricalCommand (staticcheck)

3674c33

Scratch that, use it even more.

5722017

pteichman force-pushed the pteichman/stats branch 2 times, most recently from cf7b860 to a40943a Compare May 18, 2020 17:34

Make --service-id a required argument to "stats historical"

1a13e69

pteichman force-pushed the pteichman/stats branch from a40943a to 1a13e69 Compare May 18, 2020 17:51

pteichman added 2 commits May 18, 2020 11:38

Fix JSON unmarshaling when only one service-id is in the mix

2be7eb0

Fix the tests too

a87bebf

phamann approved these changes May 18, 2020

View reviewed changes

pteichman merged commit 8489cb0 into master May 18, 2020

phamann deleted the pteichman/stats branch May 22, 2020 07:28

-	if source != manifest.SourceUndefined {
+if source == manifest.SourceUndefined {
+    return errors.ErrNoServiceID
+}
+c.Input.Service = service

Conversation

pteichman commented May 8, 2020

Uh oh!

phamann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phamann May 11, 2020

Choose a reason for hiding this comment

Uh oh!

pteichman May 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pteichman commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phamann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pteichman commented May 12, 2020

Uh oh!

mccurdyc commented May 13, 2020

Uh oh!

pteichman commented May 13, 2020

Uh oh!

phamann May 13, 2020

Choose a reason for hiding this comment

Uh oh!

pteichman May 13, 2020

Choose a reason for hiding this comment

Uh oh!

phamann May 14, 2020

Choose a reason for hiding this comment

Uh oh!

phamann left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pteichman commented May 11, 2020 •

edited

Loading