Add ability to log timings of some events...#248
Conversation
secwall
left a comment
secwall
left a comment
There was a problem hiding this comment.
I also think that we need to add tests on this.
internal/app/event_reporter.go
Outdated
| go r.send(eventType, duration) | ||
| } | ||
|
|
||
| func (r *timingReporter) send(eventType string, duration time.Duration) { |
There was a problem hiding this comment.
This approach leads to unlimited number of simultaneously running commands. Shouldn't we try a different approach? Like running a goroutine that reads a buffered channel of events and runs a single command to send them one by one?
… different downtimes)
VGoshev
force-pushed
the
add-log-timing-ability
branch
from
8810c04 to
f257da9
Compare
| command: conf.EventTimingNotifyCommand, | ||
| argsFmt: conf.EventTimingNotifyArgs, | ||
| logger: logger, | ||
| events: make(chan timingEvent, 64), // buffered channel to prevent blocking |
There was a problem hiding this comment.
64 is reused later in test. Why isn't it a constant?
| // Give worker time to process the event | ||
| time.Sleep(100 * time.Millisecond) | ||
|
|
||
| // If we reach here without panic or deadlock, the test passes |
There was a problem hiding this comment.
Are we sure this test is meaningful? We don't even check that the command was executed.
| func TestReportTimingChannelFull(t *testing.T) { | ||
| conf := &config.Config{ | ||
| EventTimingNotifyCommand: "sleep", | ||
| EventTimingNotifyArgs: []string{"10"}, // slow command to keep worker busy |
There was a problem hiding this comment.
Sleep in linux could sleep for infinity.
| // Report master unavailability duration when failover is approved | ||
| if failTime, ok := app.nodeFailTime[master]; ok { | ||
| dur := time.Since(failTime) | ||
| app.timings.reportTiming("master_unavailable", dur) |
There was a problem hiding this comment.
We probably should move all such strings into constants to ease the maintenance (imagine that we need to rename the string).
| maintenance_file: /var/run/rdsync.maintenance | ||
| daemon_lock_file: /var/run/rdsync.lock | ||
| event_timing_notify_command: "sh" | ||
| event_timing_notify_args: ["-c", "echo {event} {duration_ms} >> /var/log/rdsync_events.log"] |
There was a problem hiding this comment.
The fact that this is a command that just logs into a logfile makes me think that we could just use a separate logger instead of all this machinery. Why don't we?
2 participants
Add configurable support for logging different timings via calling of external tool.
Currently are supported following events: