Extend k6 inspect command with additional flag #2279
Conversation
| inspectOutput := interface{}(b.Options) | ||
|
|
||
| if addExecReqs { | ||
| inspectOutput, err = addExecRequirements(b, builtinMetrics, registry, logger) |
There was a problem hiding this comment.
Maybe I'm missing something but could we reuse the newRunner function from cmd/run.go and inject the runner here?
Line 390 in 5c14dae
There was a problem hiding this comment.
Yes, I started with that approach but newRunner creates a runner from loader.SourceData so it duplicates what was already done in inspect cmd, i.e. js.New / lib.ReadArchive calls. So primarily I didn't use newRunner to avoid duplication. Additionally, it just makes sense to me for NewFromBundle to be public too, even if it isn't used anywhere else right now.
|
Thanks @codebien! I've added relevant link to the description |
yorugac
added
documentation-needed
cmd/inspect.go
Outdated
| runner, err := js.NewFromBundle(logger, b, builtinMetrics, registry) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| conf, err := getConsolidatedConfig(afero.NewOsFs(), Config{}, runner) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Bundle is actually a type that I and @na-- have mostly wanted to remove in the past, so exposing it more seems like a bad idea. Refactoring everything needed is probably going to be way too much work, but I would argue you that given that you already got the Bundle, and that it actually has the options and that getConsolidatedConfig just gets the Options from the runner, which gets it from the Bundle inside .... You can just rewrite getConsolidatedConfig to be taking the Options and get the Options directly from the runner, skipping making NewFromBundle public.
Even more importantly the one thing that `get
There was a problem hiding this comment.
I think your comment got cut in two and only the first part is left 😛
Needless to say, it's the first time I'm learning of these plans... 👀 Why do you want to remove Bundle? And yes, my logic was ~ since Bundle is public, NewFromBundle can be public too.
Regardless, that's an interesting point. The main reason runner appeared here was not the getConsolidatedConfig but ExecutionScheduler. In theory it should be possible to derive the execution plan from just options without runner 🤔 I'll need to double-check if current interfaces fully allow that but at first glance that might work.
There was a problem hiding this comment.
Did you mean this: #1048 ?
There was a problem hiding this comment.
I think that issue (as most very old issues) is now very outdated, but in parts - yes.
There was a problem hiding this comment.
@mstoykov I've looked into this: trying to refactor in "minimal fashion" (without touching js/ stuff) gets stuck on the point of Runner.IsExecutable → which needs Bundle.exports so again, the same problem with private field of Bundle. Trying to refactor this at the level of Bundle is definitely too out-of-scope for this PR: I think it should be done separately.
For now, I'll add some notes on that to the #1048 issue: it can be continued in a separate PR or be left alone until next prioritization.
I added a small refactoring around getConsolidatedConfig: it can truly be used with only options, without Runner: I think it's relatively small to be present in this PR and might simplify next stages of refactorings too.
There was a problem hiding this comment.
:(
Hm looking at this again, maybe just instead of using NewBundle and then NewFromBundle, you can just go with creating the runner the same run will do. The additional work NewFromBundle does is ... minimal and should not have any side effects. But will make the cmd<->js dependency smaller.
WDYT ?
There was a problem hiding this comment.
I wouldn't say dependency would be "smaller".. meaning, even prior to this PR inspect was the only one in cmd using Bundle directly which makes sense in its case. From this follows that probably any major refactoring in js will impact inspect first.
Of course, it can be re-written as you describe but TBH, I think all options here are equally bad 😂 -- because that major refactoring is needed. I slightly prefer the current version because I don't see any harm from public NewFromBundle at this point while this version more explicitly shows the use cases that future refactoring should take into account. So it's kind of a way to document too. It may depend on how soon that refactoring will happen though.
yorugac
force-pushed
the
feat/extend-k6-inspect
branch
2 times, most recently
from
e6aef1a to
6e68f3d
Compare
yorugac
force-pushed
the
feat/extend-k6-inspect
branch
from
6e68f3d to
dc8bd69
Compare
yorugac
force-pushed
the
feat/extend-k6-inspect
branch
from
dc8bd69 to
500793f
Compare
yorugac
removed
the
documentation-needed
This PR adds additional output to
k6 inspectwith--execution-requirementsCLI option. When switched on, it triggers calculation of execution requirements, substitutes config with the derived one and addsmaxVUsandtotalDurationfields to the output.Signature of
getConsolidatedConfigwas modified: now it doesn't depend onlib.Runnerbut only onlib.Options.Some minor refactoring in
cmd/was added on the way, but not too much as that'd require additional work and likely a separate PR.This is part of the work for implementing grafana/k6-operator#9