-
Notifications
You must be signed in to change notification settings - Fork 764
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PoC: global shim for logger #4085
base: master
Are you sure you want to change the base?
Conversation
I think we want to add something about the compile time usage of this feature to the README. Mention the default, and how to compile the alternate logger in. |
@bsardo mentioned that a similar thing is done with the time package - will take a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhongshixi This is very similar to an idea you wanted to implement. Could you please review this PR instead and share your views?
.semgrep/adapter/package-import.go
Outdated
@@ -6,7 +6,7 @@ import ( | |||
// ruleid: package-import-check | |||
"github.com/mitchellh/copystructure" | |||
// ruleid: package-import-check | |||
"github.com/golang/glog" | |||
"github.com/prebid/prebid-server/v3/di" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code tests the semgrep rule. Please review the rule to check if any changes to the semgrep definition is required,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the semgrep rule seems to be checking that either of glog
or copystructure
packages is not used in the adapters, so I reverted this change to the package-import.go
.
di/di.go
Outdated
@@ -0,0 +1,8 @@ | |||
package di |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer if this is called the "logger" package, which is more intuitive. I don't know if we'd want to put all global state items in one package and currently there is only one. For simplicity, I'd like to see the interfaces and providers packages collapsed up to this level under the same "logger" package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed
di/interfaces/log.go
Outdated
@@ -0,0 +1,18 @@ | |||
package interfaces | |||
|
|||
type ILogger interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: The Java / C# naming convention does not apply to Go. It is most appropriate to name this "Logger".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed
di/providers/log/default.go
Outdated
"github.com/prebid/prebid-server/v3/di/interfaces" | ||
) | ||
|
||
type GlogWrapper struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: GlogLogger would be more direct name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed
di/providers/log/alternative.go
Outdated
@@ -0,0 +1,74 @@ | |||
//go:build custom_logger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This project does not currently use build directives to choose features. Instead, we offer a configuration system.
I like the idea of having a global log variable to override. I've seen the same in other Go projects, and it provides support for #3961. I propose to keep this ability, while also providing a config option to switch to slog from glog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This build tag is actually not required, it's just one potential mechanism that could be used to swap a dependency in compile time, so can probably removed. Configuration would require shipping both implementations and then have them swapped in runtime depending on the configuration, while compile time would only ship a single dependency. Just to be clear, by configuration - do you mean a host-level config file pbs.yaml / pbs.json right?
This PR introduces a
logger
shim and was meant to accompany a more global proposal: #4084. With this different loggers can be swapped in compile-time, however leaving the possibility to also swap a global instance in runtime (during tests f.e.)The global object is instantiated in logger/logger.go, the interface is defined in
logger/interface.go
, while implementations of loggers are inlogger/default.go
andlogger/alternative.go
.