Add plugin execution duration metric

[krissetto]

- What I did
Added utils for instrumenting plugin commands, and instrumented them to record their execution time.

Note: This is disconnected from the specific metrics and traces created by the plugins themselves and it not meant to replace those, it only represents a measurement of the binary execution of the plugin from the CLI's point of view

- How I did it

• Added some utils
• Added wrapper for exec.Cmd to be used for measuring the command's execution time
• Wired up the utils so plugin execution metrics get sent

- How to verify it

- Description for the changelog

Instrument plugin execution with otel meter

- A picture of a cute animal (not mandatory but encouraged)

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 2.73973% with 71 lines in your changes missing coverage. Please review.

Project coverage is 61.16%. Comparing base (28c5652) to head (a5644cd).
Report is 456 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5046      +/-   ##
==========================================
- Coverage   61.33%   61.16%   -0.18%     
==========================================
  Files         298      298              
  Lines       20692    20750      +58     
==========================================
  Hits        12691    12691              
- Misses       7100     7158      +58     
  Partials      901      901              

[krissetto]

The first commit is from here, for the sake of testing

[vvoland]

@krissetto Please rebase :)

[krissetto]

will fix this soon

[laurazard]

Can we just shadow dockerCli in this case instead of calling it verifiedDockerCli? Not a huge fan, sorry (I was wondering what made it "verified", and then realized it was just the typecast cli)

[krissetto]

thing is if we shadow dockerCli here, then in the else condition we can't call dockerCli.Err() as it'd be a zero value and not the original value of dockerCli. I tried avoiding this where possible (see the other places where i do this cast), but here since we need the else i wasn't sure how to proceed if not renaming the var.

Maybe there are better ways?

[thaJeztah]

Could we add a (can be un-exported) pluginInstrumenter interface and match that?

[laurazard]

Instead of creating a wrapped exec.Cmd type, how hard would it be to just have pluginmanager.PluginRunCommand return a func that starts a timer, runs the command, and ends the timer?

Okay if we end up going with a wrapper, I just try hard to avoid those/introducing more state since now it has a pointer to a cli, etc.

[krissetto]

i'm not sure that'd work..in the sense that we do things with the exec.Cmd that pluginmanager.PluginRunCommand returns aside from just running it (see the tryPluginRun() func as an example). So i wanted to keep the return value of InstrumentPluginCommand something close to what pluginmanager.PluginRunCommand currently returns. I guess we could rework all that but i'm not sure it'd be worth the extra work.

I'm not a fan of the wrapped type here either, but it seemed like a way to keep code changes relatively minimal on the usage side of things, and to allow code that doesn't need the otel bits to continue to function as usual. Happy to change the impl if we can find some simpler way to handle this. The pointer to DockerCli is really only needed to get the meter provider and the startPluginCommandTimer func atm.

The main idea of this impl was to try and keep the approach as close as possible to the one used by InstrumentCobraCommand

[laurazard]

Gotcha. Let me take a closer look, but if this is indeed the least invasive way that's okay by me. I also don't love having to remember to call a new method TimedRun, which is why I'd have rather returned a func or something.

[thaJeztah]

needs a rebase @krissetto 🙈

[laurazard]

Overall LGTM ✅

I had some nits/small comments, but I don't think they're blockers and might even be contentious, so I just pushed a branch here for comments.

Feel free to take them, I just wanted to simplify the code a bit, but we can merge this PR as-is and discuss other cleanups/refactors later.

[thaJeztah]

I think I want to have a closer look at this PR to see if we can find some alternatives for some parts 😅

[thaJeztah]

Could we add a (can be un-exported) pluginInstrumenter interface and match that?

[thaJeztah]

InstrumentPluginCommand returns a non-exported type, and we always execute TimedRun() afterwards, except for the tryPluginRun function, where we construct it, and then later execute TimedRun.

It's unfortunate that exec.Cmd doesn't implement an interface, otherwise we could've made that the return, and we can't just define an interface that only has a func Run() error

Perhaps we should either make this function perform the TimedRun; puny-code;

if cli is a pluginInstrumenter {
    cli.RunInstrumented(pluginRunCommand)
} else {
    pluginRunCommand.Run()
}

But that could even be a function, because effectively it's not a method of the CLI;

func RunPluginCommandInstrumented(cli, pluginCommand)

Hmm.. but then we get to startPluginCommandTimer, which is even more generic, but missing the baseAttrs 🤔

[krissetto]

It's unfortunate that exec.Cmd doesn't implement an interface, otherwise we could've made that the return, and we can't just define an interface that only has a func Run() error

My thoughts exactly, it's kinda annoying.

We can surely find a better way to do this, my original goal with the first implementation was to keep the approach as close as possible to that used to instrument the cobra commands, but maybe that's not a great idea all things considered. The dockerCli struct / Cli interface situation in the CLI (since we don't always pass around just the interface or just the struct) also caused some minor headaches and ugliness that would be nice to avoid if possible.