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 are missing coverage. Please review.

Project coverage is 61.16%. Comparing base (28c5652) to head (a5644cd).
Report is 4 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?

[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.