command: include default otel error handler for the cli by jsternberg · Pull Request #4975 · docker/cli

jsternberg · 2024-04-01T14:53:47Z

- What I did

This adds a default otel error handler for the cli in the debug package.
It uses logrus to log the error on the debug level and should work out
of the box with the --debug flag and DEBUG environment variable.

- How I did it

Created a function for handling the error that logs the message to logrus and registered it with this type: https://pkg.go.dev/go.opentelemetry.io/otel#ErrorHandlerFunc

- How to verify it

$ DOCKER_CLI_OTEL_EXPORTER_OTLP_ENDPOINT=://blah docker --debug info
DEBU[0000] otel error                                    error="docker otel endpoint is invalid: parse \"://blah\": missing protocol scheme"

Without this change, the output is:

2024/04/01 09:51:53 docker otel endpoint is invalid: parse "://blah": missing protocol scheme

- Description for the changelog

Include otel-related errors in debug logs.

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

Benehiko · 2024-04-02T08:23:12Z

+func init() {
+	otel.SetErrorHandler(DefaultOTELErrorHandler)
+}


I'm not a big fan of init funcs. Could we do this inside the main func instead?

Is DefaultOTELErrHandler necessary? Can't we do this instead

Suggested change

func init() {

otel.SetErrorHandler(DefaultOTELErrorHandler)

}

func init() {

otel.SetErrorHandler(otel.ErrorHandlerFunc(handleOTELError))

}

Yeah, also not a huge fan of init. If we must keep this, we could even consider inlinining handleOTELError;

Suggested change

func init() {

otel.SetErrorHandler(DefaultOTELErrorHandler)

}

func init() {

otel.SetErrorHandler(otel.ErrorHandlerFunc(func(err error) {

logrus.WithError(err).Debug("otel error")

}))

}

But I'd also be more in favour of having this up to the consumer to decide (and set the error-handler in their main or elsewhere).

+1 to not having this as an init, unless we have strong arguments for it.

(but if we must do it that way, this is one of the less awful uses of init and I'd be okay with it 😅)

The main reason is because this handler would also be standard for any plugins and I don't want to necessarily be too invasive in requiring those plugins to know to set the error handler.

I'm not a big fan of the init. I'll see if I can find a more common place to put this code.

The primary reason for DefaultOTELErrorHandler is because I also plan that this error handler is used by plugins too and I didn't want to leak the implementation. I can create the variable though and inline the function though since that won't really leak anything.

Yeah, so I was looking at this from some perspectives;

it's setting some defaults in an init, which may beccome part of imported code in other code-bases

the default is to set up a debug-logger; performing logging as part of library code is often considered a bit of a "no-no"

I'm also looking a the logrus import in this case, as I know we're trying to move away from hard-coding logrus as implementation for logging; in the moby code-base we started to use the containerd/log module (https://github.com/containerd/log), which is currently a wrapper around logrus, but could allow using that abstraction to move to a different implementation, as well as allows for implementations to attach a logger through the context (unfortunately a context-logger doesn't look to be a real option here due to the signature).

This logging is meant to tie in with the github.com/docker/cli/cli/debug package which explicitly uses logrus for the debugging. What if I were to move the error handler to there, remove the init() function, and invoke otel.SetErrorHandler on both the runDocker function and inside of one of the plugin packages?

Another option I was thinking about here could be something like this:

create an ad-hoc func (eg setDefaultOtelErrorHandler) for setting this error handler instead of init, and make sure it only executes once;

call this handler in both our "provider constructor" funcs, so:

in func (cli *DockerCli) MeterProvider(..);

and in func (cli *DockerCli) TracerProvider(..)

Tiny bit of repetition, but it'd still be transparent to plugins as long as they actually use one of the given providers, and we can avoid the init() func which would be run even when no other otel bits are being used.

WDYT?

I'm not the biggest fan of registering these with the provider constructors. It seems like invoking a function that returns a non-global value shouldn't also modify a global error handler.

I did a bit of changes and moved the error handler to the debug package that specifically uses logrus so it's easier to find and change whenever that part gets refactored. I also changed the SetErrorHandler call to be invoked from the plugin.Run function and inside of cmd/docker/main.go directly so it's no longer in an init() function. I think this might solve the problems we were discussing although I don't know if these are the best places for these calls.

codecov-commenter · 2024-04-03T14:38:42Z

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 60.95%. Comparing base (e3f45bf) to head (8f45f14).
Report is 562 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4975      +/-   ##
==========================================
- Coverage   60.99%   60.95%   -0.04%     
==========================================
  Files         295      295              
  Lines       20621    20642      +21     
==========================================
+ Hits        12578    12583       +5     
- Misses       7149     7163      +14     
- Partials      894      896       +2

thaJeztah · 2024-04-03T14:56:31Z

+
+func handleOTELError(err error) {
+	logrus.WithError(err).Debug("otel error")
+}


I think this is no longer used in the last iteration, correct?

Correct. Let me remove it.

This adds a default otel error handler for the cli in the debug package. It uses logrus to log the error on the debug level and should work out of the box with the `--debug` flag and `DEBUG` environment variable. Signed-off-by: Jonathan A. Sternberg <[email protected]>

krissetto

i'm overall ok with this. if we ever do decide to move to using the containerd/log module mentioned, we can re-think these few lines

laurazard

LGTM

Benehiko reviewed Apr 2, 2024

View reviewed changes

jsternberg force-pushed the otel-error-handler branch from 5acfb69 to 22a9635 Compare April 3, 2024 14:38

thaJeztah reviewed Apr 3, 2024

View reviewed changes

jsternberg force-pushed the otel-error-handler branch from 22a9635 to 8f45f14 Compare April 3, 2024 17:01

krissetto approved these changes Apr 3, 2024

View reviewed changes

laurazard approved these changes Apr 4, 2024

View reviewed changes

laurazard merged commit 204b324 into docker:master Apr 4, 2024

jsternberg deleted the otel-error-handler branch April 4, 2024 14:18

thaJeztah added area/metrics area/metrics/otel labels Sep 18, 2024

thaJeztah added this to the 26.1.0 milestone Sep 18, 2024

Conversation

jsternberg commented Apr 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsternberg Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsternberg Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krissetto left a comment

Choose a reason for hiding this comment

Uh oh!

laurazard left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jsternberg commented Apr 1, 2024 •

edited

Loading

jsternberg Apr 2, 2024 •

edited

Loading

jsternberg Apr 3, 2024 •

edited

Loading

codecov-commenter commented Apr 3, 2024 •

edited

Loading