-
Notifications
You must be signed in to change notification settings - Fork 712
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
Add option to print probe reports to stdout, for debugging #3204
Conversation
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.
nice simplification
probe/appclient/multi_client.go
Outdated
err := r.WriteBinary(buf, gzip.DefaultCompression) | ||
return buf, err | ||
} | ||
|
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
probe/appclient/multi_client.go
Outdated
} | ||
|
||
buf, err := ioutil.ReadAll(r) | ||
buf, err := serializeReport(r) |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
extras/fixprobe/main.go
Outdated
@@ -60,8 +62,12 @@ func main() { | |||
log.Fatal(err) | |||
} | |||
|
|||
rp := appclient.NewReportPublisher(client, false) | |||
buf := &bytes.Buffer{} | |||
err = fixedReport.WriteBinary(buf, gzip.DefaultCompression) |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
prog/main.go
Outdated
@@ -274,6 +275,7 @@ func setupFlags(flags *flags) { | |||
flag.Bool("app-only", false, "Only run the app.") | |||
|
|||
// Probe flags | |||
flag.BoolVar(&flags.probe.printOnStdout, "probe.print-on-stdout", false, "Print reports on stdout instead of sending to app, for debugging") |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
prog/probe.go
Outdated
handle.Canonical = true | ||
return codec.NewEncoder(os.Stdout, handle).Encode(rep) | ||
} | ||
|
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
prog/probe.go
Outdated
publisher = new(stdoutPublisher) | ||
} else { | ||
publisher = clients | ||
} |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
I have made all the suggested changes, with the exception of changing the commit message to clarify the removed "optimisation". I should squash commits so can fix the message then. |
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.
Nice!
Please do some squashing.
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.
lgtm
Simplification: everything now implements Publish(Report), and we do away with writing/reading/writing in the MultiAppClient.
Simplification: move the 'noControls' functionality into the probe, as we don't need a whole struct to do that. The ReportPublisher interface also moves into probe where it belongs: "the consumer should define the interface" - Dave Cheney
Move the creation of the buffer and the choice of compression level (which never changes) into WriteBinary(), to simplify the code.
This makes the next code change easier to see.
This option gives a crude way to view the raw probe data as json in the container logs, so that you can check exactly what it would have sent. We stub out the PipeClient interface with a dummy implementation in this mode.
f1631bb
to
80dbd34
Compare
Squashed and rebased |
It's fairly common that, while debugging, I need to check whether the probe sent the wrong thing or the app rendered the wrong thing. This option gives a crude way to view the raw probe data as json in the container logs.
It wasn't immediately obvious how to add it, so I removed two layers of cruft from the app client, and then it was easy.
A small fix-up to the
fixprobe
utility was also needed.