Injectable logging implementation #3204
Conversation
|
Honestly not sure what to do with regards to tests. Obviously fixed all tests where this would cause problems, but should we test logging itself somehow? I've just started the server with and without this to and verified it looks the same. |
anderseknert
force-pushed
the
SDK-logging
branch
from
a560cb1 to
ac32a2a
Compare
anderseknert
force-pushed
the
SDK-logging
branch
from
1bd2500 to
7835179
Compare
anderseknert
force-pushed
the
SDK-logging
branch
from
7835179 to
2b7388b
Compare
plugins/bundle/plugin.go
Outdated
| @@ -47,10 +48,12 @@ type Plugin struct { | |||
| // New returns a new Plugin with the given config. | |||
| func New(parsedConfig *Config, manager *plugins.Manager) *Plugin { | |||
| initialStatus := map[string]*Status{} | |||
| loggers := make(map[string]sdk.Logger, len(parsedConfig.Bundles)) | |||
There was a problem hiding this comment.
Can we avoid instantiating multiple loggers here? We're going to run into issues if the plugin gets reconfigured to download different bundles. We could address that by instantiating new loggers on reconfiguration, but I don't think that's worth it.
A single logger that allows a set of key-value pairs to be passed to the log entry would be enough here.
plugins/bundle/plugin_test.go
Outdated
| @@ -41,6 +43,7 @@ func TestPluginOneShot(t *testing.T) { | |||
| bundleName := "test-bundle" | |||
| plugin.status[bundleName] = &Status{Name: bundleName, Metrics: metrics.New()} | |||
| plugin.downloaders[bundleName] = download.New(download.Config{}, plugin.manager.Client(""), bundleName) | |||
| plugin.loggers[bundleName] = sdk.NewStandardLogger() | |||
There was a problem hiding this comment.
Nit: Having to instantiate loggers everywhere the plugin is intantiated is a suboptimal. If we exposed logging functions that accepted logger parameters, we could check for nil in the logging function and avoid this kind of thing.
There was a problem hiding this comment.
Yeah this would be avoided if these tests instantiated via the New constructor.. but 116da7809afbed2344a7c13bb8aa440eea928203 removes the need for this even if constructed this way.
| ) | ||
|
|
||
| // Level log level for Logger | ||
| type Level uint8 |
anderseknert
force-pushed
the
SDK-logging
branch
from
2b7388b to
116da78
Compare
Refactor logging to allow providing custom logging implementations to plugin manager. This should allow us to keep logging as it is when running OPA as a server, while injecting noop-loggers or custom, provided loggers for SDK client implementations. Fixes open-policy-agent#3180 Signed-off-by: Anders Eknert <anders@eknert.com>
anderseknert
force-pushed
the
SDK-logging
branch
from
116da78 to
faff6f8
Compare
Refactor logging to allow providing custom logging implementations to plugin
manager. This should allow us to keep logging as it is when running OPA as a
server, while injecting noop-loggers or custom, provided loggers for SDK client
implementations.
Fixes #3180
Signed-off-by: Anders Eknert anders@eknert.com