export RunStat, StatsOptions, and add Filters option

[ndeloof]

The filter option is not currently exposed on the command-line,
but can be added as a flag in future. It will be used by compose
to filter the list of containers to include based on compose
labels.

- What I did
introduce --filter flag to select containers to be observed
export RunStat and StatOptions so this command can be shared with Docker Compose to implement new stats command (docker/compose#11234)

- How I did it

- How to verify it

- Description for the changelog
// only for internal purpose, not expected to be "public" as a public API

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

[codecov-commenter]

Codecov Report

Merging #4685 (8f617ad) into master (3b57acb) will increase coverage by 0.36%.
The diff coverage is 0.00%.

❗ Current head 8f617ad differs from pull request most recent head 7548f78. Consider uploading reports for the commit 7548f78 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4685      +/-   ##
==========================================
+ Coverage   59.26%   59.63%   +0.36%     
==========================================
  Files         285      287       +2     
  Lines       24748    24774      +26     
==========================================
+ Hits        14668    14774     +106     
+ Misses       9194     9114      -80     
  Partials      886      886              

[thaJeztah]

I'm slightly confused here; should the --filter option apply to the events, or should the filtering be used for listing the containers? (getContainerList below)?

For compose, I could imaging we want to get a list of containers that are part of the compose file, or does compose pass a list of container ID's to look at?

[thaJeztah]

(I guess I may have to look at all logic more closely)

[ndeloof]

you're right, filter must also apply to getContainerList
passing --filter label=compose.docker.compose.project=xx allows to only select containers and events related to a compose project (also could reduce scope to only focus on a service)

[thaJeztah]

Was considering if we should make the "getContainerList" function something that can be passed in. Currently it's a bit of an odd combination of either passing in a hard-coded list of containers, or "all" containers, which uses that function.

If we would pass that function in as argument, perhaps that would allow compose (or other consumers) to do whatever logic they want to get the list of containers.

But honestly, haven't given that much thought yet (or looked what that would look like)

[ndeloof]

the benefits I can see in this function is that this "magically" includes new container created after command was started as long as they match the criteria. So consumer only has to establish a filter (label-based, could also filter by name or status?)

[thaJeztah]

We should probably provide a link to the docker ps filter section (as I think that's the filters that would be available) However see my other comment below.

[thaJeztah]

OK, found a small bit of time to check this PR (but have not yet looked into "possible alternatives"). I found a regression though, so we need to look into that.

I'm also curious; is the intent to expose a --filter flag on the compose side? Or is the filter primarily meant to be used "internally" by compose (so that it can filter based on compose's "internal" labels (com.docker.compose.project=XXX))?

I had a quick look at what the UX would be like, and at a quick glance, I think we should look at that part more closely; the combination of --filter AND positional arguments ([CONTAINER...]) may be somewhat confusing. We already have some issues there (e.g. the --all option), but perhaps we should look closer before we commit to introducing the option.

Here's some testing I did;

docker run -d --name one --label hello=world nginx:alpine
docker run -d --name two --label hello=goodbye nginx:alpine
docker run -d --name three --label hello=hallo nginx:alpine

This looks like regression; running docker stats without arguments does not produce an output, and does not refresh; the cli "hangs" with no output;

 ./build/docker stats

I was trying the above, as I was curious what the UX is when running docker stats without passing a container, and a filter that doesn't match any containers.

./build/docker stats --filter label=hello=nomatch

Also tried with a filter that should match a container, but also hangs (no output);

 ./build/docker stats --filter label=hello=world

Run docker stats for a specific container AND pass a filter that should not match that container, but it looks like in that case the filter is ignored;

./build/docker stats --filter label=nomatch one

CONTAINER ID   NAME      CPU %     MEM USAGE / LIMIT     MEM %     NET I/O       BLOCK I/O         PIDS
49db05bcc543   one       0.00%     12.81MiB / 7.663GiB   0.16%     1.02kB / 0B   7.77MB / 12.3kB   11

Given that --filter here accepts the same filters as docker ps, there may be combinations that can be confusing https://docs.docker.com/engine/reference/commandline/ps/#filter

For example;

exited: An integer representing the container's exit code. Only useful with --all.
status: One of created, restarting, running, removing, paused, exited, or dead

The --all would also apply here as well (not sure if that would be immediately obvious to the user; IMO we should also look at that for docker ps for a slightly better UX)

[ndeloof]

My intent was for the filter to primarily be used "internally" by compose to filter containers by project label, but I had the feeling it could be useful for this command to select a subset of resources, same as docker ps does

[thaJeztah]

My intent was for the filter to primarily be used "internally" by compose to filter containers by project label, but I had the feeling it could be useful for this command to select a subset of resources, same as docker ps does

Perhaps, based on the above, it'd be best to move that change separate; would you be able to move that change to a separate PR, so that we could move this one forward for v25?

(also if you could check my examples, as I think some parts didn't work as expected, so there could be a bug at hand)

[ndeloof]

sure, updated to drop public --filter flag until we get some consensus on such a feature

[thaJeztah]

Did a quick rebase and amended the commit message to remove the --filter mention, but it looks like there's still something wrong; with this build, docker stats (without arguments) hangs, and doesn't show stats;

./build/docker stats

With the v24.0 CLI (same daemon, same state);

CONTAINER ID   NAME                              CPU %     MEM USAGE / LIMIT     MEM %     NET I/O           BLOCK I/O         PIDS
af4c1c63924e   zen_payne                         0.00%     8.098MiB / 7.663GiB   0.10%     2.54MB / 30.4kB   0B / 1.68MB       11
3e6d3e37862a   foo.1.wm6sej6mfaldkgb51me88rrww   0.00%     8.066MiB / 7.663GiB   0.10%     6.7kB / 0B        0B / 12.3kB       11
1f2ed82385d3   foobar                            0.00%     9.148MiB / 7.663GiB   0.12%     8.87kB / 0B       4.56MB / 442kB    11
c0d124d2465d   mypostgres                        0.01%     21.36MiB / 7.663GiB   0.27%     23.4kB / 0B       1.46MB / 57.6MB   6
477f387c7224   nomatch                           0.00%     6.906MiB / 7.663GiB   0.09%     23.5kB / 0B       0B / 1.24MB       11
a01e96c62d11   three                             0.00%     6.926MiB / 7.663GiB   0.09%     23.6kB / 0B       0B / 1.25MB       11
c5969117edfe   two                               0.00%     7.066MiB / 7.663GiB   0.09%     23.6kB / 0B       0B / 1.23MB       11
49db05bcc543   one                               0.00%     9.129MiB / 7.663GiB   0.12%     23.7kB / 0B       7.77MB / 1.23MB   11

[thaJeztah]

Okay, I dug into this a bit, and I found the issue, but not sure yet how to resolve the problem and make filtering work as expected.

While looking at the code, I also did some refactoring / cleanup; I'll open a separate PR for that (currently included in this branch).

[thaJeztah]

Hm... perhaps I was looking in the wrong direction; events allow filtering by label (so we have some mapping behind the scenes), and so does container list (which we call for the initial list of containers).

Perhaps it's due to both using the same filter, and these lines causing the filter to be updated, causing the docker container list to try to filter on "type";

f := options.Filter.Value()
f.Add("type", string(events.ContainerEventType))

[thaJeztah]

Yup, that's it; the container-list API call fails, which gets sent to the closeChannel, but doesn't surface anywhere; if I print the error, it shows;

./build/docker stats
Error response from daemon: invalid filter 'type'

[thaJeztah]

I fixed this one, and it's now working, but I made some changes for the initial implementation;

• the Filters option is now a pointer (to make it more explicitly "optional"; leave it nil to not use a filter)
• I changed the Filters type back to a filters.Args; that is the filter type "only", without the added features to make it directly usable for a cobra Flag. I did so trying to avoid it being too tightly coupled to "command-line", and it's easy to use an intermediate option for that purpose if needed later.
• currently, passing BOTH a list of containers (IDs/Names) AND a filter produces an error (we currently don't support that combination)
• due to how the filters are used, we can only use the subset of filters supported by_both_ container list AND docker events. We can somehow find out what is supported by making additional API calls, but I wanted to limit complexity for now, so I hard-coded the list of supported filters to label only (for now). I THINK Compose is currently interested in label only for filtering, but we could add additional ones if we really need it immediately.

[thaJeztah]

'LGTM"

last touch-ups were done by me, but I got an "LGTM" on slack