feat(logging): Allow registering callbacks for logging events #17916
Conversation
telegraf-tiger
bot
added
the
feat
|
Does this addition also add possibility to implement #10245? |
Co-authored-by: Thomas Casteleyn <[email protected]>
Yes it will be possible with the heartbeat plugin I think. |
|
That plugin only limits the errors to be sent to a HTTP post endpoint. The request mentioned was more general. (e.g. extra option in inputs.internal to convert/emit each log message as a new metric, where it then could be processed as desired..) |
logger/logger.go
Outdated
| // Serve all registered callbacks | ||
| callbackMu.Lock() | ||
| for _, cb := range callbacks { | ||
| cb(level, ts.UTC(), l.source, l.attributes, args...) | ||
| } |
There was a problem hiding this comment.
Callbacks are executed before the log level check, so they run for all messages regardless of the configured level. Consider moving the callback execution after the level check to avoid unnecessary calls. If the heartbeat plugin intentionally needs all messages, that should be clearly documented.
Also, note that the mutex is held while executing all callbacks:
callbackMu.Lock()
for _, cb := range callbacks {
cb(level, ts.UTC(), l.source, l.attributes, args...) // user code runs with lock held!
}
callbackMu.Unlock()If a callback is slow or blocks, it will block all logging operations. It might be worth documenting this behavior or switching to a copy-then-execute pattern:
callbackMu.Lock()
cbs := append([]CallbackFunc(nil), callbacks...)
callbackMu.Unlock()
for _, cb := range cbs {
cb(level, ts.UTC(), l.source, l.attributes, args...)
}There was a problem hiding this comment.
Changed the mutex to an rw-mutex which should resolve the issue... Should I execute the callbacks in a go-routine to prevent blocking the logging?
There was a problem hiding this comment.
Sticking with synchronous makes sense; the RWMutex already allows concurrent logging calls, so multiple goroutines can call Print() and execute callbacks concurrently.
If the heartbeat plugin needs to do any slow work (like HTTP calls), it should handle that asynchronously within its own callback using a channel/queue pattern rather than blocking the logger.
There was a problem hiding this comment.
I also feel like the callbacks should happen after level checking. It might give too much unwanted/undocumented behaviour if not?
For example, it will become very hard in the callback to have Debug level for plugin instance A and Warning level for plugin instance B, while that logic is already present.
There was a problem hiding this comment.
@Hipska I think exactly the contrary is the case if you put the callbacks after filtering because then you will have a combination of all logging configuration with the filtering potentially done on the receiver side of the callback as you basically do a "which-ever setting is higher" kind of operation. That prevents having different filtering for file-based logging and the plugin use.
I agree that having the filtered logs would also be valuable but we should then add another set of callbacks called after the filtering basically allowing to have custom logging outputs that behave the same way as file/console-based logging...
Yeah that would either go with the |
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 👍 This pull request doesn't change the Telegraf binary size 📦 Click here to get additional PR build artifactsArtifact URLs |
4 participants
Summary
This PR allows to register callbacks for logging events to be able to get informed about such events. Those callbacks are required for the heartbeat plugin.
Checklist
Related issues
related to #17577