[receiver/dockerstats] support pids_stats metrics #21111

sigilioso · 2023-04-24T13:51:58Z

Description:

Adds support for pids_stats metrics.

pids_stats metrics were introduced in docker engine API v1.23 and that's the reason why docker's API default version has been bumped to 1.23. However, the minimum supported version has not been updated because the metrics are optional and reported only when pids cgroup is supported and the kernel is >= 4.3. On top of that, pids_stats may be reported even if the API request is performed using previous versions. Example:

curl --unix-socket /var/run/docker.sock -H "Content-Type: application/json" http://1.22/containers/78de07328afff50a9777b07dd36a28c709dffe081baaf67235db618843399643/stats?stream=false |jq
{
(...)
  "pids_stats": {
    "current": 1,
    "limit": 2192
  },
(...)
}

Link to tracking Issue: #21041

Testing:

ScarapeV2 test has been updated:

Expectation files for the previous tests now include pids.count metric (already being there in the corresponding stats json files)
Included new ScrapeV2 test case where pids_stastmetrics are not reported.
Included new ScrapeV2 tests case where both new metrics container.pids.count and container.pids.max are reported.

Tests where executed as follows:

❯ make test
go test -race -timeout 300s -parallel 4 --tags="" ./...
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver  0.805s
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver/internal/metadata        0.500s

❯ go test --tags integration ./...
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver  33.664s
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver/internal/metadata        0.404s

Documentation:
New metrics documentation was generated using mdatagen

receiver/dockerstatsreceiver/metadata.yaml

jamesmoessis · 2023-04-26T01:42:56Z

receiver/dockerstatsreceiver/receiver.go

@@ -126,6 +126,7 @@ func (r *receiver) recordContainerStats(now pcommon.Timestamp, containerStats *d
 	r.recordMemoryMetrics(now, &containerStats.MemoryStats)
 	r.recordBlkioMetrics(now, &containerStats.BlkioStats)
 	r.recordNetworkMetrics(now, &containerStats.Networks)
+	r.recordPidsMetrics(now, &containerStats.PidsStats)


What is the behaviour on API 1.22, or <=4.3 kernel? I would like to ensure there is no nil pointer crash. I would expect it just does not record the metric. But I can't see any explicit handling of these cases

Since PidStats is not defined as a pointer in docker types it will be an empty struct, therefore this pointer cannot be nil even if pids_stats are not reported.

The stats response in the new test case pids_no_stats does not include pids_stats metrics, showing this use-case.

jamesmoessis · 2023-04-26T01:53:24Z

receiver/dockerstatsreceiver/receiver_test.go

+	})
+	assert.NoError(t, err)
+	defer pidsStatsMaxEngineMock.Close()
+


Not strictly related to your features, but I think we should probably refactor the test case struct so it defines the engine mocks. Otherwise these tests are going to get messy as more testcases are added.
Like this:

mockDockerEngine: func () *httptest.Server { mockServer, err := dockerMockServer(...) assert.NoError(t, err) return mockServer }()

Also you may need to loop over the test cases, and defer mockServer.Close() (before t.Run()) to ensure they all get closed.

Thanks for the suggestion! I didn't consider changing the test structure but it makes a lot of sense. I moved the mock definition inside the test case struct as suggested. Since it is in a function and the server is created inside of each t.Run I think it is safe closing it inside as well. Please, let me know your thoughts.
Changed here: f9b58f3

Thanks for improving that, it looks good.

jamesmoessis · 2023-04-26T01:57:17Z

receiver/dockerstatsreceiver/receiver_test.go

+	assert.NoError(t, err)
+	defer noPidsStatsEngineMock.Close()
+
+	pidsStatsMaxEngineMock, err := dockerMockServer(&map[string]string{


How does this test case differ from the existing test cases? You've added the pid stats into those too (which is good). Is it possible we can remove this test case, or does it provide something different to the others?

In this test case both metrics are returned in the stats response:

"pids_stats": { "current": 1, "limit": 2192 },

(limit is only reported if a limit is defined, when it is unlimited it is not reported)

Then, the corresponding expectation file also include both metrics.

On the other hand, the existing test cases' stats response returned the current value only, which is the only value added in the expectation file.

receiver/dockerstatsreceiver/metadata.yaml

jamesmoessis · 2023-04-26T02:14:07Z

receiver/dockerstatsreceiver/metadata.yaml

@@ -628,3 +628,19 @@ metrics:
      aggregation: cumulative
    attributes:
      - interface
+
+  # Pids
+  container.pids.count:


Related to my comment about the semantic convention: also consider the naming of the metric and try and somewhat align it to the semantic convention. Do you think container.processes.count is better for alignment? I'll let you make that call, but it's something to consider.

Apparently processes is more widely used and is more aligned with the semantic convention, but taking into account that the value is named pids in cgroups and docker API does not change it, maybe keeping pids here makes these new metrics easier to identify and understand. WDYT?

Yeah, I tend to agree. It's reasonable to keep it as pids here.

jamesmoessis · 2023-04-26T23:27:59Z

receiver/dockerstatsreceiver/metadata.yaml

+      aggregation: cumulative
+      monotonic: false
+
+  container.pids.max:


Is there a reason you've gone with "max" as the terminology here? Seeing as docker and cgroups report it as "limit" it might make sense for the metric name to be container.pids.limit. This is also consistent with other metrics in this component such as container.memory.usage.limit.

I took it from cgroups docs:

In order to use the pids controller, set the maximum number of tasks in pids.max (this is not available in the root cgroup for obvious reasons). The number of processes currently in the cgroup is given by pids.current.

But, as you stated, it is reported as limit and that name would be consistent with other metrics and the conventions, so I'm changing it. Thanks again! 🙂

Changed here: 653ad44

jamesmoessis

Looks good to me!

Depending on whether #21166 gets merged before this, you may have some conflicts to resolve.

jamesmoessis · 2023-05-10T02:56:44Z

@codeboten as the codeowner this looks good - just needs approver/maintainer review.

receiver/dockerstatsreceiver/receiver.go

sigilioso requested review from a team and codeboten April 24, 2023 13:51

github-actions bot assigned codeboten Apr 24, 2023

github-actions bot added the receiver/dockerstats label Apr 24, 2023

github-actions bot requested review from jamesmoessis and rmfitzpatrick April 24, 2023 13:52

jamesmoessis reviewed Apr 26, 2023

View reviewed changes

sigilioso force-pushed the include-pids-stats branch from 74f6e9a to f9b58f3 Compare April 26, 2023 08:58

sigilioso requested a review from jamesmoessis April 26, 2023 15:48

jamesmoessis reviewed Apr 26, 2023

View reviewed changes

sigilioso requested a review from jamesmoessis April 27, 2023 06:34

jamesmoessis approved these changes Apr 28, 2023

View reviewed changes

jamesmoessis mentioned this pull request Apr 28, 2023

[receiver/dockerstats] Add optional resource attributes #21185

Merged

sigilioso force-pushed the include-pids-stats branch from 653ad44 to 9a877df Compare May 4, 2023 06:58

sigilioso added 5 commits May 8, 2023 16:55

feat(dockerstatsreceiver): support pids_stats metrics

e8e1862

chore: fix typo

a8e4bb0

chore: update metrics type to follow semantic conventions

c094ff8

chore: scrape test refactor

352ef06

chore: improve metric naming

bfe1278

sigilioso force-pushed the include-pids-stats branch 2 times, most recently from 75946c9 to 9d71853 Compare May 9, 2023 06:15

chore: fix rebase issues

3f67fc1

sigilioso force-pushed the include-pids-stats branch from 9d71853 to 3f67fc1 Compare May 9, 2023 06:31

sigilioso and others added 3 commits May 10, 2023 15:11

chore: fix conflicts

1205f9c

Merge branch 'open-telemetry:main' into include-pids-stats

42c8b64

Merge branch 'main' into include-pids-stats

35fe5e4

atoulme reviewed May 19, 2023

View reviewed changes

receiver/dockerstatsreceiver/receiver.go Show resolved Hide resolved

atoulme approved these changes May 19, 2023

View reviewed changes

atoulme added the ready to merge Code review completed; ready to merge by maintainers label May 19, 2023

codeboten merged commit f61d138 into open-telemetry:main May 19, 2023

github-actions bot added this to the next release milestone May 19, 2023

crobert-1 mentioned this pull request Jun 7, 2023

[receiver/dockerstats] Add pids_stats metrics #21041

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[receiver/dockerstats] support pids_stats metrics #21111

[receiver/dockerstats] support pids_stats metrics #21111

sigilioso commented Apr 24, 2023

jamesmoessis Apr 26, 2023

sigilioso Apr 26, 2023

jamesmoessis Apr 26, 2023

sigilioso Apr 26, 2023 •

edited

Loading

jamesmoessis Apr 26, 2023

jamesmoessis Apr 26, 2023

sigilioso Apr 26, 2023 •

edited

Loading

jamesmoessis Apr 26, 2023

sigilioso Apr 26, 2023 •

edited

Loading

jamesmoessis Apr 26, 2023

jamesmoessis Apr 26, 2023

sigilioso Apr 27, 2023

sigilioso Apr 27, 2023

jamesmoessis left a comment •

edited

Loading

jamesmoessis commented May 10, 2023

[receiver/dockerstats] support pids_stats metrics #21111

[receiver/dockerstats] support pids_stats metrics #21111

Conversation

sigilioso commented Apr 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigilioso Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigilioso Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigilioso Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesmoessis left a comment • edited Loading

Choose a reason for hiding this comment

jamesmoessis commented May 10, 2023

sigilioso Apr 26, 2023 •

edited

Loading

sigilioso Apr 26, 2023 •

edited

Loading

sigilioso Apr 26, 2023 •

edited

Loading

jamesmoessis left a comment •

edited

Loading