debug logger: remove logger callbacks #209
Conversation
| // values. | ||
| var regionNames map[ProductRegion]string | ||
| // debugMutex protects access to product debug region/enabled information. | ||
| var debugMutex sync.RWMutex |
There was a problem hiding this comment.
Renamed the globals to clearly indicate that they are only for debug log handling.
| "sync" | ||
| ) | ||
|
|
||
| // ProductRegion is a numerical value assigned to an area of the product. |
There was a problem hiding this comment.
Added docs throughout the file.
| // startup. | ||
| func RegisterRegions(regions map[ProductRegion]string) { | ||
| regionNames = regions | ||
| debugMutex.Lock() |
There was a problem hiding this comment.
Used the mutex for ALL debugRegions and debugEnabled accesses. We no longer assume that some functions are only called at program startup. Consistent use of the mutex is easier to explain that implicit conventions.
| regionsEnabled[region] = true | ||
| regionName := RegionName(region) | ||
| debugEnabled[region] = true | ||
| regionName := debugRegions[region] |
There was a problem hiding this comment.
InitDebugLogs() directly accesses debugRegions because RegionName() now uses debugMutex; we need InitDebugLogs() to hold the mutex until its work is complete.
All debug logger instances were previously attached to a global set of callbacks. This was done so loggers could be informed if they were ever enabled dynamically. Unfortunately, loggers were never removed from this set of callbacks, which meant that those loggers could never be reclaimed by the garbage collector. Debug loggers are often constructed as the program runs, adding fields and values to assist with tracing. This meant that programs often did not have a static set of loggers, but were always creating new debug loggers, which were always seen as live objects. Now, debug loggers always ask their enabled status from the map tracking enabled state. This incurs the cost of a mutex-read lock and map lookup at runtime.
aronatkins
force-pushed
the
aron-leaky-debug
branch
from
19f1595 to
e395fd4
Compare
| // | ||
| // Equivalent to `debugLogger.WithField("sub_region", subregion)` | ||
| func (l *debugLogger) WithSubRegion(subregion string) DebugLogger { | ||
| newLgr := l.Logger.WithField("sub_region", subregion) |
There was a problem hiding this comment.
This isn't really relevant for this PR but I don't understand why WithSubRegion exists. Why not just call WithFields() like we do in the tests down below?
gr.WithFields(Fields{
"region": "alfa",
"sub_region": "drinks",
})There was a problem hiding this comment.
I do not want to alter the public API.
There was a problem hiding this comment.
The API exists because folks wanted a well-known field name.
All debug logger instances were previously attached to a global set of callbacks. This was done so loggers could be informed if they were ever enabled dynamically. Unfortunately, loggers were never removed from this set of callbacks, which meant that those loggers could never be reclaimed by the garbage collector. Debug loggers are often constructed as the program runs, adding fields and values to assist with tracing. This meant that programs often did not have a static set of loggers, but were always creating new debug loggers, which were always seen as live objects.
Now, debug loggers always ask their enabled status from the map tracking enabled state. This incurs the cost of a mutex-read lock and map lookup at runtime.
This change is associated with Connect issue 29616