Issue 558740043: Adds the agent uptime field to the agent pulse. (Closed)
|
Created:
4 years, 11 months ago by alanmorales Modified:
4 years, 11 months ago Reviewers:
akaiser CC:
opt-crs_google.com Visibility:
Public. |
DescriptionAdds the agent uptime field to the agent pulse.
Uses the ticker to determine the number of milliseconds it should send.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Calculating uptime using simple time.Now() #
Total comments: 1
MessagesTotal messages: 12
Please take a look, thanks!
Sign in to reply to this message.
https://codereview.appspot.com/558740043/diff/552670043/agent/control/pulse.go File agent/control/pulse.go (right): https://codereview.appspot.com/558740043/diff/552670043/agent/control/pulse.g... agent/control/pulse.go:108: uptimeMs := ps.statsTracker.TickCount() * int64(pulseFrequency/time.Millisecond) The usage of the statsTracker seems overly complicated for this design. Why not just have a "startTime time.Time" as part of the PulseSender struct, which is set when the PulseSender is first created in NewPulseSender, and then in pulseMsg() do uptime := time.Now().Sub(ps.startTime).Nanoseconds() / 1000000 That will leave the statsTracker entirely out of the equation. https://codereview.appspot.com/558740043/diff/552670043/proto/pulse.proto File proto/pulse.proto (right): https://codereview.appspot.com/558740043/diff/552670043/proto/pulse.proto#new... proto/pulse.proto:27: int64 agent_uptime_ms = 7; Missing comment "// Time in millis since agent startup"
Sign in to reply to this message.
On 2019/05/08 20:48:24, akaiser wrote: > https://codereview.appspot.com/558740043/diff/552670043/agent/control/pulse.go > File agent/control/pulse.go (right): > > https://codereview.appspot.com/558740043/diff/552670043/agent/control/pulse.g... > agent/control/pulse.go:108: uptimeMs := ps.statsTracker.TickCount() * > int64(pulseFrequency/time.Millisecond) > The usage of the statsTracker seems overly complicated for this design. Why not > just have a "startTime time.Time" as part of the PulseSender struct, which is > set when the PulseSender is first created in NewPulseSender, and then in > pulseMsg() do > > uptime := time.Now().Sub(ps.startTime).Nanoseconds() / 1000000 > > That will leave the statsTracker entirely out of the equation. > > https://codereview.appspot.com/558740043/diff/552670043/proto/pulse.proto > File proto/pulse.proto (right): > > https://codereview.appspot.com/558740043/diff/552670043/proto/pulse.proto#new... > proto/pulse.proto:27: int64 agent_uptime_ms = 7; > Missing comment "// Time in millis since agent startup" I guess I'm mostly concerned with the subtractions / division being more expensive than a multiplication - as well as potential complications with testing; i think this would involve implementing a mock clock. If you feel strongly towards using an approach where the direct times are used, I'm open to change it.
Sign in to reply to this message.
On 2019/05/08 22:06:58, alanmorales wrote: > On 2019/05/08 20:48:24, akaiser wrote: > > https://codereview.appspot.com/558740043/diff/552670043/agent/control/pulse.go > > File agent/control/pulse.go (right): > > > > > https://codereview.appspot.com/558740043/diff/552670043/agent/control/pulse.g... > > agent/control/pulse.go:108: uptimeMs := ps.statsTracker.TickCount() * > > int64(pulseFrequency/time.Millisecond) > > The usage of the statsTracker seems overly complicated for this design. Why > not > > just have a "startTime time.Time" as part of the PulseSender struct, which is > > set when the PulseSender is first created in NewPulseSender, and then in > > pulseMsg() do > > > > uptime := time.Now().Sub(ps.startTime).Nanoseconds() / 1000000 > > > > That will leave the statsTracker entirely out of the equation. > > > > https://codereview.appspot.com/558740043/diff/552670043/proto/pulse.proto > > File proto/pulse.proto (right): > > > > > https://codereview.appspot.com/558740043/diff/552670043/proto/pulse.proto#new... > > proto/pulse.proto:27: int64 agent_uptime_ms = 7; > > Missing comment "// Time in millis since agent startup" > > I guess I'm mostly concerned with the subtractions / division being more > expensive than a multiplication - as well as potential complications with > testing; i think this would involve implementing a mock clock. If you feel > strongly towards using an approach where the direct times are used, I'm open to > change it. Subtraction and division are trivially cheap computations, I wouldn't optimize for that cost right now. In general, optimize for simplicity and readability, and to that end I'd recommend using direct times.
Sign in to reply to this message.
Please take another look, using time.Now() to calculate agent uptime.
Sign in to reply to this message.
LGTM This is much cleaner, thank you. I had on comment about the units, but I'll let you decide whether you want to address it or not. https://codereview.appspot.com/558740043/diff/572680043/agent/control/pulse.go File agent/control/pulse.go (right): https://codereview.appspot.com/558740043/diff/572680043/agent/control/pulse.g... agent/control/pulse.go:119: AgentUptimeMs: int64(time.Now().Sub(ps.startTime).Seconds() * 1000), I won't block your change on this, but if we're only ever going to provide second granularity then we should have made the AgentUptime proto message in seconds rather than milliseconds. I'd be really surprised if "time.Now().Sub(ps.startTime).Nanoseconds() / 1000000" ever had any measurable performance impact on the Agent, especially considering this is a calculation we perform once every ten seconds. If this is a super performance critical loop then worring about a division makes sense, but for something that happens so infrequently in "human time", IMO this is a premature optimization.
Sign in to reply to this message.
On 2019/05/09 17:28:37, akaiser wrote: > LGTM > This is much cleaner, thank you. I had on comment about the units, but I'll let > you decide whether you want to address it or not. > > https://codereview.appspot.com/558740043/diff/572680043/agent/control/pulse.go > File agent/control/pulse.go (right): > > https://codereview.appspot.com/558740043/diff/572680043/agent/control/pulse.g... > agent/control/pulse.go:119: AgentUptimeMs: > int64(time.Now().Sub(ps.startTime).Seconds() * 1000), > I won't block your change on this, but if we're only ever going to provide > second granularity then we should have made the AgentUptime proto message in > seconds rather than milliseconds. > > I'd be really surprised if "time.Now().Sub(ps.startTime).Nanoseconds() / > 1000000" ever had any measurable performance impact on the Agent, especially > considering this is a calculation we perform once every ten seconds. If this is > a super performance critical loop then worring about a division makes sense, but > for something that happens so infrequently in "human time", IMO this is a > premature optimization. Sounds good, I actually think .Seconds returns a floating point which includes miliseconds (the go playground also seems to return floating points, indicating fractions of second) so it should really give more than second per second precision. I've added a comment to clarify this.
Sign in to reply to this message.
On 2019/05/09 17:36:25, alanmorales wrote: > On 2019/05/09 17:28:37, akaiser wrote: > > LGTM > > This is much cleaner, thank you. I had on comment about the units, but I'll > let > > you decide whether you want to address it or not. > > > > https://codereview.appspot.com/558740043/diff/572680043/agent/control/pulse.go > > File agent/control/pulse.go (right): > > > > > https://codereview.appspot.com/558740043/diff/572680043/agent/control/pulse.g... > > agent/control/pulse.go:119: AgentUptimeMs: > > int64(time.Now().Sub(ps.startTime).Seconds() * 1000), > > I won't block your change on this, but if we're only ever going to provide > > second granularity then we should have made the AgentUptime proto message in > > seconds rather than milliseconds. > > > > I'd be really surprised if "time.Now().Sub(ps.startTime).Nanoseconds() / > > 1000000" ever had any measurable performance impact on the Agent, especially > > considering this is a calculation we perform once every ten seconds. If this > is > > a super performance critical loop then worring about a division makes sense, > but > > for something that happens so infrequently in "human time", IMO this is a > > premature optimization. > > Sounds good, I actually think .Seconds returns a floating point which includes > miliseconds (the go playground also seems to return floating points, indicating > fractions of second) so it should really give more than second per second > precision. I've added a comment to clarify this. SGTM. I just looked it up and the Seconds() function has a division in it too: https://golang.org/src/time/time.go?s=25511:25546#L796
Sign in to reply to this message.
On 2019/05/09 17:40:42, akaiser wrote: > On 2019/05/09 17:36:25, alanmorales wrote: > > On 2019/05/09 17:28:37, akaiser wrote: > > > LGTM > > > This is much cleaner, thank you. I had on comment about the units, but I'll > > let > > > you decide whether you want to address it or not. > > > > > > > https://codereview.appspot.com/558740043/diff/572680043/agent/control/pulse.go > > > File agent/control/pulse.go (right): > > > > > > > > > https://codereview.appspot.com/558740043/diff/572680043/agent/control/pulse.g... > > > agent/control/pulse.go:119: AgentUptimeMs: > > > int64(time.Now().Sub(ps.startTime).Seconds() * 1000), > > > I won't block your change on this, but if we're only ever going to provide > > > second granularity then we should have made the AgentUptime proto message in > > > seconds rather than milliseconds. > > > > > > I'd be really surprised if "time.Now().Sub(ps.startTime).Nanoseconds() / > > > 1000000" ever had any measurable performance impact on the Agent, especially > > > considering this is a calculation we perform once every ten seconds. If this > > is > > > a super performance critical loop then worring about a division makes sense, > > but > > > for something that happens so infrequently in "human time", IMO this is a > > > premature optimization. > > > > Sounds good, I actually think .Seconds returns a floating point which includes > > miliseconds (the go playground also seems to return floating points, > indicating > > fractions of second) so it should really give more than second per second > > precision. I've added a comment to clarify this. > > SGTM. I just looked it up and the Seconds() function has a division in it too: > https://golang.org/src/time/time.go?s=25511:25546#L796 Mildly interesting, but I was interested in seeing if the compiler would compile Nanos() / 10000 and Seconds() * 1000 differently or if it would effectively detect that they were the same. Turns out they're mostly the same except for an extra MOV instruction when using Seconds() ^.^ (but in fact, the arithmetic is the same) In case you're interested: https://drive.google.com/file/d/1bOnwK5ujwrDJEUK4HM1PWQMu1xwmfjN4/view?usp=sh...
Sign in to reply to this message.
On 2019/05/09 17:40:42, akaiser wrote: > On 2019/05/09 17:36:25, alanmorales wrote: > > On 2019/05/09 17:28:37, akaiser wrote: > > > LGTM > > > This is much cleaner, thank you. I had on comment about the units, but I'll > > let > > > you decide whether you want to address it or not. > > > > > > > https://codereview.appspot.com/558740043/diff/572680043/agent/control/pulse.go > > > File agent/control/pulse.go (right): > > > > > > > > > https://codereview.appspot.com/558740043/diff/572680043/agent/control/pulse.g... > > > agent/control/pulse.go:119: AgentUptimeMs: > > > int64(time.Now().Sub(ps.startTime).Seconds() * 1000), > > > I won't block your change on this, but if we're only ever going to provide > > > second granularity then we should have made the AgentUptime proto message in > > > seconds rather than milliseconds. > > > > > > I'd be really surprised if "time.Now().Sub(ps.startTime).Nanoseconds() / > > > 1000000" ever had any measurable performance impact on the Agent, especially > > > considering this is a calculation we perform once every ten seconds. If this > > is > > > a super performance critical loop then worring about a division makes sense, > > but > > > for something that happens so infrequently in "human time", IMO this is a > > > premature optimization. > > > > Sounds good, I actually think .Seconds returns a floating point which includes > > miliseconds (the go playground also seems to return floating points, > indicating > > fractions of second) so it should really give more than second per second > > precision. I've added a comment to clarify this. > > SGTM. I just looked it up and the Seconds() function has a division in it too: > https://golang.org/src/time/time.go?s=25511:25546#L796 Mildly interesting, but I was interested in seeing if the compiler would compile Nanos() / 10000 and Seconds() * 1000 differently or if it would effectively detect that they were the same. Turns out they're mostly the same except for an extra MOV instruction when using Seconds() ^.^ (but in fact, the arithmetic is the same) In case you're interested: https://drive.google.com/file/d/1bOnwK5ujwrDJEUK4HM1PWQMu1xwmfjN4/view?usp=sh...
Sign in to reply to this message.
On 2019/05/10 16:06:46, alanmorales wrote: > On 2019/05/09 17:40:42, akaiser wrote: > > On 2019/05/09 17:36:25, alanmorales wrote: > > > On 2019/05/09 17:28:37, akaiser wrote: > > > > LGTM > > > > This is much cleaner, thank you. I had on comment about the units, but > I'll > > > let > > > > you decide whether you want to address it or not. > > > > > > > > > > https://codereview.appspot.com/558740043/diff/572680043/agent/control/pulse.go > > > > File agent/control/pulse.go (right): > > > > > > > > > > > > > > https://codereview.appspot.com/558740043/diff/572680043/agent/control/pulse.g... > > > > agent/control/pulse.go:119: AgentUptimeMs: > > > > int64(time.Now().Sub(ps.startTime).Seconds() * 1000), > > > > I won't block your change on this, but if we're only ever going to provide > > > > second granularity then we should have made the AgentUptime proto message > in > > > > seconds rather than milliseconds. > > > > > > > > I'd be really surprised if "time.Now().Sub(ps.startTime).Nanoseconds() / > > > > 1000000" ever had any measurable performance impact on the Agent, > especially > > > > considering this is a calculation we perform once every ten seconds. If > this > > > is > > > > a super performance critical loop then worring about a division makes > sense, > > > but > > > > for something that happens so infrequently in "human time", IMO this is a > > > > premature optimization. > > > > > > Sounds good, I actually think .Seconds returns a floating point which > includes > > > miliseconds (the go playground also seems to return floating points, > > indicating > > > fractions of second) so it should really give more than second per second > > > precision. I've added a comment to clarify this. > > > > SGTM. I just looked it up and the Seconds() function has a division in it too: > > https://golang.org/src/time/time.go?s=25511:25546#L796 > > Mildly interesting, but I was interested in seeing if the compiler would compile > Nanos() / 10000 and Seconds() * 1000 differently or if it would effectively > detect that they were the same. Turns out they're mostly the same except for an > extra MOV instruction when using Seconds() ^.^ (but in fact, the arithmetic is > the same) > > In case you're interested: > https://drive.google.com/file/d/1bOnwK5ujwrDJEUK4HM1PWQMu1xwmfjN4/view?usp=sh... Neat. I keep forgetting that Seconds() returns a float, so it isn't necessarily losing any precision.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
