[image-inspect]: stdout/stderr and logging refactor

[saehejkang]

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Motivation and Context

Relates to #642.

• Each image carries it own error message (allows for different error types per image)
• Stdout/stderr support
• Use of the new StderrLogHandler

container image inspect test python:alpine
// Inspect output for python:alpine here but omitted due to size
test: The operation couldn’t be completed. (ContainerizationError.ContainerizationError error 1.)
Error: InspectError(succeeded: ["docker.io/library/python:alpine"], failed: [("test", notFound: "Image not found")])

I found these updates make error propogation/handling a lot easier. If this is the correct direction we want to take, I will start by making updates to all the image commands before continuing on #832.

Testing

• Tested locally
• Added/updated tests
• Added/updated docs

[jglogan]

Thanks for kicking this off!

I'm going to be pretty tied up today but I'll take a look at it later or tomorrow. There's #642 which you mentioned and also #268 which lacks actionable work but we should look it over and see if we can close that with the set of fixes like this.

[Ronitsabhaya75]

@saehejkang Throwing InspectError at the end causes swift-argument-parser to print the raw struct details (e.g. Error: InspectError(...)) to stderr.

Since you are already logging specific errors via Logger, this final error print is redundant and noisy.

My Suggestion: Throw ExitCode.failure instead of InspectError to exit non-zero without extra noise, OR make InspectError conform to LocalizedError with a clean errorDescription.

[saehejkang]

@Ronitsabhaya75 My initial thought process was to make InspectError conform to LocalizedDescription but that would make more sense if we are under the assumption there is a single type of error message ("image not found"). That way also introduces a bit more complexity to define more error types and descriptions. By using a single custom error type, we keep error handling simple and consistent, throwing only one type of error. The raw struct may be noisy but at least we have control on the logging, as well as user facing errors.

[Ronitsabhaya75]

@saehejkang I see your point about keeping the implementation simple! However, we can actually eliminate the noisy raw struct print without adding complex error types.

If InspectError conforms to LocalizedError and returns a generic summary, swift-argument-parser will print that instead of the raw struct dump.

extension ImageInspect.InspectError: LocalizedError {
    var errorDescription: String? {
        // We already logged specific errors above, so a simple summary suffices here.
        return "Failed to inspect \(failed.count) image(s)."
    }
}

This keeps the error handling logic exactly as you have it but makes the CLI output much cleaner for the end user.

[jglogan]

@saehejkang Did InspectError not get added to the PR? I only see the inspect command in the commit.

[jglogan]

@saehejkang @Ronitsabhaya75 also see the PR I pushed for creating a ManagedResource protocol. What do you think about having ContainerResource or ContainerAPIClient define an error type for ManagedResource that can wrap ContainerizationError?, and then we use those the typical resource access errors instead of using ContainerizationError for some and defining resource-specific, overlapping errors for others (VolumeError)?

I won't do any error-related work in that PR (I'm going to force push a rewrite of what I have, made a lot of changes), so feel free to play around in this one and we can work out something coordinated.

[saehejkang]

Did InspectError not get added to the PR? I only see the inspect command in the commit.

https://github.com/apple/container/pull/1044/files#diff-f331141407ca78f7d4e5608fc336b134bd416ba8897a22bce745820cf45807f0R39:~:text=)%20%7B%7D-,struct,-InspectError%3A%20Error

What do you think about having ContainerResource or ContainerAPIClient define an error type for ManagedResource that can wrap ContainerizationError?

I like the idea of having a single ManagedResource type that can wrap ContainerizationError. I did take a look at the VolumeError and realized that all the other resources were out of date or did not follow the same design pattern. Different commands/resource, follow different patterns, so on and so forth.

If we follow one simple pattern, that could alleviate issues I was running into. For example, in the ClientImage.get() here, updating the return to be error: [ImageError] (with .notFound or .duplicate error codes) could be more useful. This can be then read back by ImageInspect and the correct errors can be filtered and thrown.

However, there could be another discussion on even if commands should even be throwing ANY errors. Should they only be thrown in the client/server functions?

[jglogan]

However, there could be another discussion on even if commands should even be throwing ANY errors. Should they only be thrown in the client/server functions?

Yeah, one model I'd had in my head was that the API service and client would throw errors that were principally data carriers with minimal message text, and any code that uses the API client is responsible for taking the data and presenting it as it saw fit.

That's "informed" by my ignorance of how localization works for errors; that model assumes that the caller of the client is best equipped to localize errors.

It does feel less fussy if the errors thrown by the API server and client are ready to present without any additional work on the caller's part.

What do you think about a PartialError type that's a holder for a wrapped collection of errors from a compound operation? The PartialError meaning would be "it's up to you to determine whether this constitutes a proper error".

[jglogan]

@saehejkang Also make sure to rebase to pick up the test fix!

[jglogan]

I also just pushed #1066 that gives all the commands a logger that outputs the message and metadata only at levels other than debug and trace, and includes an ISO timestamp at those levels.

What I'd like to do is use structured logging principles for all our CLI logging. I'd written up these guidelines elsewhere and think they're applicable here:

Use the following guidelines when crafting log messages:

• Write concise and legible messages. The message should be a short, complete sentence that describes the event being logged. Avoid unnecessary jargon.
• Do not interpolate variables into log messages. Instead, use structural logging to add fields to messages. Choose meaningful field names, using the conventional field names below for commonly occurring data so that operations can be tracked readily across applications.
• Use contextual logging to apply the same fields to a range of messages. If a context consists of identifying information and attributes, do not overload the context with the attributes. Instead, place the identifying information in the context, and insert an initial log message that includes the additional attributes.
• Exclude sensitive information from log messages. Pay special attention when logging values in data structures, classes, and collections - it is generally safer to log only that which is needed, instead of sending such objects to the logger.
• Reduce clutter by avoiding informational logging in frequently called methods, and when iterating large collections.
• For languages that do not support conditional evaluation of log messages, test whether log levels are enabled before invoking logging messages that require expensive computation. Swift uses autoclosures to condition log message evaluation, but when using a language like Java, you should enclose expensive messages in code like: log.isDebugEnabled() {...}.

Conventional Structured Logging Fields

Name	Description
container	A container name.
image	An imageref value.
count	Result set size.
rc	Exit status for a command.
request_millis	Request processing time in milliseconds.
url	A request URL.
version	The application version.

[Ronitsabhaya75]

@jglogan i'll have look once I'm good after my interview next week