Open na-- opened 1 year ago
If you want to test with a branch of k6 where lib.State.Logger
is the interface and not the concrete type, see https://github.com/grafana/k6/pull/2960
As you can see, I had to temporarily remove xk6-browser to be able to compile k6 and run the tests :disappointed:
Btw, I just realized, you don't actually need to run the tests with https://github.com/grafana/k6/pull/2960 :sweat_smile: Since the *logrus.Logger
in lib.State
implements the logrus.FieldLogger
interface, if you switch to the interface on the xk6-browser side, your code should work both with the k6 master
branch and the k6 logrus-refactor
branch, right?
Hey, @na--,
I'm first currently working on the logrus-refactor
branch before coming up with a better solution from the xk6-browser side. I've tried to switch to the FieldLogger
interface, however, it didn't go well as I expected since we need many methods from the *Logger
:
SetReportCaller
as we don't use it often.SetLevel
and GetLevel
to enable or disable logs when needed to debug the extension (as it is quite hairy to do so).
verbose
option to enable debug logs. However, we use the debug and trace levels.SetFormatter
to create a Logger
used when displaying console.log
events with the exact times as they are sent from the browser.
As a hack, we can accept a FieldLogger
in logger.New
, and internally type assert it to *logrus.Logger
and use that until k6-core changes it to something else. The following passes the tests on the logrus-refactor
branch:
diff --git a/vendor/github.com/grafana/xk6-browser/log/logger.go b/vendor/github.com/grafana/xk6-browser/log/logger.go
index 722aae96..43f6eda4 100644
--- a/vendor/github.com/grafana/xk6-browser/log/logger.go
+++ b/vendor/github.com/grafana/xk6-browser/log/logger.go
@@ -31,14 +31,18 @@ func NewNullLogger() *Logger {
}
// New creates a new logger.
-func New(logger *logrus.Logger, iterID string) *Logger {
+func New(logger logrus.FieldLogger, iterID string) *Logger {
var defLogger bool
if logger == nil {
defLogger = true
logger = logrus.New()
}
+ ll, ok := logger.(*logrus.Logger)
+ if !ok {
+ // panic so that we know when to upgrade
+ panic("logger is not a logrus.Logger")
+ }
l := &Logger{
- Logger: logger,
+ Logger: ll,
iterID: iterID,
}
if defLogger {
On the sidelines, I'm also thinking of a better solution than applying the type-assertion hack.
WithFields
method and using the returned Entry
. However, we'll still be depending on the logrus :( Then again, it could be a better solution than the current one.FieldLogger
interface with the methods we need. Not ideal either :(*log.Logger
instead of depending on *logrus.Logger
and calling the related methods of the FieldLogger
interface. Still, this doesn't solve the third point above (needing to know the output destination—it seems we can get it via the WithFields
) :(PS: I ran the k6 tests to see what happens with the behavior between xk6-browser removed and added. I don't want to introduce anything unexpected 🤞 That's why I asked whether they are known as flaky :)
Hey, @na--! I can make k6-browser remain compatible with k6-core using the hack solution I suggested in my previous comment. Otherwise, we don't have enough capacity to keep working on this in this cycle 😒 And we'll be happy working on it in the next cycle with a proper solution.
Sorry for not replying earlier. Yeah, let's move forward with the hack, so we can do this step by step. I had to do a similar hack here, since I didn't have time to refactor newFileConsole()
. I will do that in a separate future PR, since newFileConsole()
has a whole bunch of other problems, e.g. using os.OpenFile()
directly...
So, please go ahead with this, and maybe open a separate issue for the remaining work, or we can keep this one open afterwards. In the long run, we want to get rid of logrus completely, but that is definitely a very long-term project. We can figure out how to refactor the SetLevel()
and GetLevel()
and SetFormatter()
dependence later.
Hey @na--, I've created another issue to apply the hack: #823. As we discussed, we can keep this one open and tackle it in the next cycle.
Please see https://github.com/grafana/k6/issues/2958
After some investigation, it seems like it won't be that difficult to get rid of the
*logrus.Logger
concrete type in k6 in favor of thelogrus.FieldLogger
interface :tada:However, what is currently stopping me is the usage of
*logrus.Logger
by xk6-browser, specifically: https://github.com/grafana/xk6-browser/blob/48ee41a59e0f0561dbcae39b91dc6fba338422a4/log/logger.go#L18 https://github.com/grafana/xk6-browser/blob/48ee41a59e0f0561dbcae39b91dc6fba338422a4/log/logger.go#L34I am not sure why you need all of that complexity in your logger, but because you expect
vu.State.Logger
to be the specific type and not the interface, I can't fix this on the k6 side without breaking this vendored xk6-browser code: https://github.com/grafana/xk6-browser/blob/748ae91092fd05083d1eaaac8594dd7de808d2ce/common/response.go#L74