performance: send active controls as a single string per node #3714
Conversation
Instead of a whole extra data structure which is quite expensive to marshal and unmarshal, just send the information in a string. No clever merging strategy is required - the states are all set in one place per node type.
Makes report.go a little easier to read.
…obes With a test.
e4660b0 to
635cea0
Compare
report/node.go
Outdated
| n.LatestControls = n.LatestControls.Set(control, ts, data) | ||
| return n | ||
| // ActiveControls returns a string slice with the names of active controls. | ||
| func (n Node) ActiveControls(cs ...string) []string { |
There was a problem hiding this comment.
cs doesn't seem to be used in the function or am I missing something? Weird how linter didn't complain:
| func (n Node) ActiveControls(cs ...string) []string { | |
| func (n Node) ActiveControls() []string { |
There was a problem hiding this comment.
Right, it was bad copy/paste from the method above, thanks.
Go doesn't complain about unused parameters inside the function because they may be needed to conform to an interface. And it's ok to supply an empty list when calling a ... function.
| if !v.Value.Dead { | ||
| cs = append(cs, name) | ||
| // Pull out the newest timestamp to use for the whole set | ||
| if ts.Before(v.Timestamp) { |
There was a problem hiding this comment.
Is this always true before ts is set? Because otherwise we should make sure it doesn't end up being nil I guess?
There was a problem hiding this comment.
ts is initialized to the "zero value" for time.Time which compares as before any real time.
ts cannot be nil as it is a value not a pointer type.
| "docker_remove_container": { | ||
| "timestamp": "2019-10-14T14:36:01Z", | ||
| "value": {"dead": false} | ||
| } |
There was a problem hiding this comment.
Could you add a couple more entries here? e.g. some with the same control name but different timestamps and some with differing control names for a more complete test?
There was a problem hiding this comment.
I added a little bit more but not sure if I understand you.
Fix a logic error in ECS scale-down button, bad copy/paste in ActiveControls() and neaten the switch cases in container controls. Co-Authored-By: Filip Barl <filip@weave.works>
d16610e to
1dcdfab
Compare
Scope "controls" are buttons to do something like restart a container.
These controls can be turned on and off by the probe, e.g. depending on whether the container is running.
Previously this information was conveyed by a complicated data structure mapping each control name to a
Deadboolean which had a last-modified timestamp. All of which takes effort to send and merge.This PR changes the representation to a single string saying which controls are active, e.g.
"pause;restart;stop". String values have a timestamp, so the latest value prevails in a merge.I guess we have to update the plugins docs but haven't really looked.
This is a protocol change, requires a version bump.