config: add HeartbeatUpdateInterval#4832
Conversation
winder
left a comment
winder
left a comment
There was a problem hiding this comment.
I guess the only harm would be if everyone changes this to 10s and we end up with a lot more data in the DB. But heartbeat events are probably a small fraction of the total data, so it probably wouldn't be a problem.
Codecov Report
@@ Coverage Diff @@
## master #4832 +/- ##
==========================================
+ Coverage 53.64% 54.02% +0.38%
==========================================
Files 432 420 -12
Lines 54058 53803 -255
==========================================
+ Hits 28997 29066 +69
+ Misses 22813 22363 -450
- Partials 2248 2374 +126
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
cce
force-pushed
the
config-add-heartbeatupdateinterval
branch
from
60321f1 to
23f0074
Compare
|
@winder addressed your comment in #4832 (review) |
| case cfg.HeartbeatUpdateInterval < 60: // min frequency 1 minute | ||
| interval = time.Minute |
There was a problem hiding this comment.
Could this be an error instead? Might be easier than having to document / explain why the units change depending on how big the number is.
There was a problem hiding this comment.
Alternatively, the units could be minutes, that way 1 is just the smallest non-disabled value.
There was a problem hiding this comment.
the units don't change ... they are still seconds..?
There was a problem hiding this comment.
being converted into a time.Duration
There was a problem hiding this comment.
Ah, yes thats more reasonable. I read that as time.Minute * interval
There was a problem hiding this comment.
It could still be confusing. I'm imagining a local test where I set it to 5 to get some more information, then have to switch tasks to figure out why the events aren't being sent out.
There was a problem hiding this comment.
I am assuming no one wants to set it lower than a minute, that it is more valuable to prevent this type of usage, and that user would use the /metrics endpoint instead
There was a problem hiding this comment.
nit: I think changing units to minutes would be a slightly better option than changing the config at runtime
There was a problem hiding this comment.
I only chose it to match the telemetry update interval configuration units that were already being used:
// PeerConnectionsUpdateInterval defines the interval at which the peer connections information is being sent to the
// telemetry ( when enabled ). Defined in seconds.
PeerConnectionsUpdateInterval int `version[5]:"3600"`
Co-authored-by: Will Winder <wwinder.unh@gmail.com>
3 participants
Summary
Adds a configurable HeartbeatUpdateInterval to control how often the heartbeat telemetry event is sent, when telemetry is enabled.
Test Plan
Existing tests should pass.