WIP: Add more testing + memory stats from cgroups #10

hamiltont · 2020-02-03T06:34:04Z

PR adds:

Reports unit memory metrics by reading memory.stat from cgroupfs
Pulls cgroup code into distinct package to prep for upstreaming
Improves TravisCI setup - example
- Adds end-to-end integration test for two live systemd versions
- Adds unit tests
Increases scrape speed by about 15% by reducing dbus calls
Uploads code coverage - example

Remaining TODOs

Decide what memory stats we should export. There are a lot. @povilasv any thoughts you have would be appreciated here. It seems silly to start exporting 20 new metrics per unit so a decision should be made w.r.t. what memory metrics are most impactful
Cleanup docs
Cleanup cgroup/cgroup.go file

fixes #2

Looking into current systemd source code, this value only increases. e.g. https://github.com/systemd/systemd/blob/7c286cd6a615fa9bce8a2830133bcf89becfbf9f/src/core/socket.c#L2416

Main problem is that this does not support test coverage, because we are using an external binary as the binary-under-test and coverage works by compiling a custom binary with coverage-tracking-logic when running go test. It's not simple to force compilation of these tracking statements, so as long as we use an external binary we cannot have coverage data. Minor issue is that the process launch time is relatively slow. Not an issue when running 2-3 tests, but this is may be an issue down the road

This is a minimal proof-of-concept for how to test the exporter while also getting code coverage details. Should also be a bit faster. Bad things: If something ever goes wrong, cleaning up would be much harder because we cannot just kill the external process. So arguably this could be a bad thing, or in the future we bother to split up testing into smoke tests that can run in this manner and larger tests that should be run as an external process. But for now I'm happy with this, I like seeing code coverage reports :-) While it's tempting to think this magically enables parallelism in the test suite, remember we are sharing the global default http server here. It would not be too hard to change that, and also auto-update the port numbers, but it's overkill for now. I'll just run stuff serially until it becomes an issue

Replaces the 'call an external binary' approach with the new 'call a handler function in main.go' approach. Much faster. This commit also shows a few example test cases just to get the ball rolling

Their focus on testing-inside-docker means they do not have a good testing-in-vm experience. Only one ubuntu version is supported, and upgrading it on each CI run is both wasteful and a huge pain to get correct (we only want to upgrade golang, but they have a customized version of ubuntu to the typical backported PPA approach does not work out of the box). As a nice side bonus, travis-ci is mostly OSS while circleCI is not, so we can read the source if we have any issues with travis.

See golangci/golangci-lint#658 for details Summary - golangci-lint, built with Go 1.12, will generate this error when linting Go 1.13 code

povilasv · 2020-02-06T07:06:03Z

Hey 👋 let me know when it's ready for review :)

hamiltont · 2020-02-06T07:17:38Z

@povilasv Sure, anytime now should be OK. This PR is big already (sorry, seems to be my bad habit), so let's aim to clean it up as needed and get it merged before the next push ;-) I could use any insight you have on "what memory values matter enough to export as metrics"

hamiltont · 2020-02-06T07:33:59Z

Also, a second concern - what should the exported metrics be named? node_exporter moved to using the pattern node_memory_Capname_metric (see screenshot from one of my prometheus consoles). I could not find a good discussion on why they changed their naming pattern, or why they export so many metric names instead of just having something like node_memory{stat="anon"}. Regardless, this is a good time to consider how systemd_exporter wants timeseries for memory metrics to be organized

povilasv · 2020-02-10T14:33:04Z

Will review this week thanks for this :)

Re naming I am not sure why node exporter does this but I think we should follow https://prometheus.io/docs/practices/naming/ and do whatever makes sense for our use case and query patterns.

I feel like doing stat=anon will make one metric be super big which will suffer from huge cardinality explosion so maybe a metric per stat

@SuperQ Would be great to get some insights from you regarding naming :)

SuperQ · 2020-02-10T15:06:19Z

Labels only make sense when there is an aggregation that can be applied. Using a label for node_memory_... doesn't make sense because the various values don't add up or average together. For example: sum(node_memory_bytes) doesn't make any sense since MemTotal + MemAvailable bytes is nonsensical.

Having separate metrics also helps for more typical use cases like this:

node_memory_MemAvailable_bytes /
node_memory_MemTotal_bytes

If it were a label, you would have to do something crazy like this:

sum without (stat) (node_memory_bytes{stat="MemAvailable"}) /
sum without (stat) (node_memory_bytes{stat="MemTotal"})

hamiltont · 2020-02-10T16:07:04Z

That's very helpful, thanks. We should export distinct metrics then. @povilasv to answer the second question (what should we export) maybe we just expose the same metrics that node_exporter does, but do it per unit being monitored. It would be more than enough to get anyone started. Thoughts? Side point - we may want to document that users may want to run more than one systemd_exporter with different units monitored at different levels to deal with cardinality issues. I've got an idea on this for future, thinking we could allow folks to filter by the slice or scope units are in

baryluk · 2020-02-10T19:22:08Z

My opinion on metrics selection. I do not see harm on essentially exporting all of them. If they are constant, they will be well compressed by Prometheus and consume almost no resources.

The more interesting question would be which metrics to keep as a separate metric/gauge, and which ones to put together with separate labels. I think it is useful to have some metrics grouped into single gauge with separate labels, if it does make sense to 1) visualise all of them at the same time, 2) make sum of them to see for a total of something.

From a quick look it looks like this only applies to few metrics.

rss and rss_huge could be put together. Unless rss from cgroups already accounts for huge allocations.
faults. it might make sense to use single counter with minor and major label values. But I do expect major to be what most people look at, with minor providing almost no information, and being significantly bigger often, leading to hiding of actual information. So that might still be beneficial to keep them separate.

As of the others metrics, I think they should be all separate (because doing aggregation over them doesn't make much sense), but I didn't look too deeply into this, and there might be some exceptions.

SuperQ · 2020-02-10T20:11:37Z

Thanks @baryluk

One thing to point out, is if we have metrics that have parts where they can be summed, and we also have a "total" exposed as another part, we try and avoid exposing those in Prometheus best practices. This allows cleaner use of sum(metric).

baryluk · 2020-02-10T20:21:54Z

Thanks @baryluk

One thing to point out, is if we have metrics that have parts where they can be summed, and we also have a "total" exposed as another part, we try and avoid exposing those in Prometheus best practices. This allows cleaner use of sum(metric).

Sounds good!

Following Prometheus best practices is a very good idea, to reduce amount of user confusion, but in general it is actually very logical, short and well written to avoid common mistakes with naming metrics and labels.

However regarding your comment, I can't find a general guideline in Prometheues best practices about not exporting total metric if sum(other_metric) provides same information (modulo minor atomicity differences and moving the aggregation cost from one process to another, and adding a bit extra of memory / storage).

I checked https://prometheus.io/docs/practices/naming/

cgroup/cgroup.go

povilasv · 2020-02-13T10:30:01Z

cgroup/cgroup.go

+// path appends the given path elements to the filesystem path, adding separators
+// as necessary.
+func (fs FS) path(p ...string) string {
+	return filepath.Join(append([]string{string(fs.mountPoint)}, p...)...)


this is a bit much, consider just using flepath.Join(fs.MountPoint, subpath, suffix) directly in https://github.com/povilasv/systemd_exporter/pull/10/files#diff-966caeb64fc8c233e22788d6e3caccbbR171 and
https://github.com/povilasv/systemd_exporter/pull/10/files#diff-966caeb64fc8c233e22788d6e3caccbbR173

povilasv · 2020-02-13T10:31:42Z

cgroup/cgroup.go

+	// if cgroupUnified != unifModeUnknown {
+	// 	return cgroupUnified, nil
+	// }


Let's get rid of commented out code

povilasv · 2020-02-13T11:00:12Z

cgroup/cgroup.go

+
+	switch fs.Type {
+	case cgroup2SuperMagic:
+		log.Debugf("Found cgroup2 on /sys/fs/cgroup/, full unified hierarchy")


As this is a package, consider not logging or allowing users to choose their own logging pkg, more info https://dave.cheney.net/2015/11/05/lets-talk-about-logging

cgroup/cgroup_test.go

povilasv · 2020-02-14T07:06:00Z

main.go

+
+}
+
+func testMain(wg *sync.WaitGroup) *http.Server {


Consider moving this into testing package.

povilasv · 2020-02-14T07:08:18Z

main_test.go

+	// b, err := ioutil.ReadAll(resp.Body)
+	// if err != nil {
+	// 	return nil, err
+	// }
+	// if err := resp.Body.Close(); err != nil {
+	// 	return nil, err
+	// }
+	// if want, have := http.StatusOK, resp.StatusCode; want != have {
+	// 	return nil, fmt.Errorf("want /metrics status code %d, have %d. Body:\n%s", want, have, b)
+	// }


Let's remove commented out code

povilasv · 2020-02-14T07:12:07Z

systemd/systemd_test.go

+	if found != "service" {
+		t.Errorf("Bad unit name parsing. Wanted %s got %s", "service", found)
+	}
+


Let's get rid of empty space

povilasv · 2020-02-14T07:14:19Z

systemd/systemd.go

+			break
+		}
+		err = c.collectUnitCPUMetrics(*cgroupPath, conn, ch, unit)
+		if err != nil {


Suggested change

if err != nil {

if err != nil && parseUnitType(unit) != "socket" {

povilasv · 2020-02-14T07:14:32Z

systemd/systemd.go

+		if err != nil {
+			// Most sockets do not have a cpu cgroupfs entry, but a few big ones do (notably docker.socket). Quiet down
+			// error reporting if error came from a socket
+			if parseUnitType(unit) != "socket" {


Suggested change

if parseUnitType(unit) != "socket" {

SuperQ · 2020-02-14T14:02:58Z

@baryluk It's under the labels section of "writing exporters".

https://prometheus.io/docs/instrumenting/writing_exporters/#labels

Exports the ability to manually create a new cgroup filesystem struct, as well as the various mount modes. Cleans up documentation

hamiltont · 2020-02-24T18:31:18Z

Retitled as WIP while I work through the feedback

hectorhuertas · 2020-04-29T15:25:16Z

@hamiltont thanks for all the amazing work you've done here. do you have plans to resume work on this PR? those cgroup rss metrics are very appealing :)

hamiltont · 2020-04-29T16:52:11Z

Thanks! Yes, I actually have some nice commits to push, been busy with my day job. I'll try to get out an update this week

…

On Wed, Apr 29, 2020, 11:25 AM Hector Huertas ***@***.***> wrote: @hamiltont <https://github.com/hamiltont> thanks for all the amazing work you've done here. do you have plans to resume work on this PR? those cgroup rss metrics are very appealing :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACKRZFBAUJ42BRO5BJ2S4DRPBBGZANCNFSM4KO7X5JA> .

talyz · 2021-09-13T21:18:07Z

What's the status here? I've been trying this out for a while and it really makes the metrics much more useful. I had to add a small patch to ignore services with the RemainAfterExit property (talyz@c4f6128) in order for it to not spam my logs, but that's the only issue I've had with it so far.

hamiltont · 2021-09-13T22:08:26Z

Well unfortunately real life jumped up and took hold, but I am thrilled to hear that some folks found it useful At this point.... If anyone has time to wrap this work up (as I recall that's mainly responding to the feedback that was given) it would be super appreciated to have the help I'll see if I can push all the other improvements that I had to a distinct branch in case anyone is interested. As I recall they were definitely not ready (I was/am learning golang in the evenings, so while I can see the concepts needed to make a system better it takes me quite a while to translate those into useful go code)

…

On Mon, Sep 13, 2021, 5:18 PM Kim Lindberger ***@***.***> wrote: What's the status here? I've been trying this out for a while and it really makes the metrics much more useful. I had to add a small patch to ignore services with the RemainAfterExit property ***@***.*** <talyz@284e372>) in order for it to not spam my logs, but that's the only issue I've had with it so far. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACKRZBGEXT4IBTHVN4FRVLUBZTBVANCNFSM4KO7X5JA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

talyz · 2021-09-14T15:54:20Z

I see :) Yes, it's really useful - great work!

Unfortunately, I don't think I can be of much help - I only know golang at a very basic level. I hope someone can, though.

anarcat · 2022-05-10T15:43:54Z

i filed bug #46 about how memory usage is reported incorrectly by this exporter. would this PR fix that problem?

SuperQ · 2022-07-19T14:55:11Z

After a very long time, we've moved this to prometheus-community. If you're still interested in working on this, please rebase this change against the latest commits.

oseiberts11 · 2022-11-02T17:18:44Z

I tried to "rebase this change against the latest commits." but there were a lot of merging problems, but the result as it is (squashed, because there were simply way too many conflicts in the commit-by-commit rebase) is in #66 .

hamiltont added 30 commits January 30, 2020 10:02

Change socket refused from gauge to counter

c6abe3d

Looking into current systemd source code, this value only increases. e.g. https://github.com/systemd/systemd/blob/7c286cd6a615fa9bce8a2830133bcf89becfbf9f/src/core/socket.c#L2416

Fix counter-or-gauge metric documentation

36d4121

Markdown cleanup

d4c73f7

Cleanup test suite and use mainTest by default

abb9906

Replaces the 'call an external binary' approach with the new 'call a handler function in main.go' approach. Much faster. This commit also shows a few example test cases just to get the ball rolling

Ensure tests run server at proper address

5a700f6

Fix testing bugs - run main & avoid hang on --version

0ad0613

First circleCI image

ee24c67

Testing artifacts and machine builds

324b61f

Add workflow

b1152d1

Stop using container at all. Start using BASH_ENV

161855d

Stop attempting bashrc hacks

a70a1e6

Initial travis CI file

8f9e075

Workaround bug in golangci-lint

26df7d2

See golangci/golangci-lint#658 for details Summary - golangci-lint, built with Go 1.12, will generate this error when linting Go 1.13 code

Fix bug found by ci linting :-)

7c9b7c8

Added codecov.io

cd0bfae

Define CI matrix directly (no job include)

10a20d3

Update test flags to work with codecov

4804ae2

Including integration tests into coverage counts

13ca4f3

Listing units in travis-ci

4c9259f

Log args as debug to aid in testing

3bb0ed0

Testing

4060136

New go-acc version to work with go.mod

d0e740e

Debugging - add go list

44c22fa

Document todo regarding integration test coverage

17652d4

Enabling cgroup accounting on Travis-CI

62fe927

Fixing typo

37f6d68

Documenting requirements for CPUAccounting

1a6ae16

Accept linting suggestion

6884b2a

povilasv mentioned this pull request Feb 6, 2020

Fixup metrics for socket_refused_connections #8

Open

hamiltont changed the title ~~WIP: Add more testing + memory stats from cgroups~~ Add more testing + memory stats from cgroups Feb 6, 2020

hamiltont requested a review from povilasv February 9, 2020 17:49

povilasv reviewed Feb 14, 2020

View reviewed changes

cgroup: Export multiple new symbols

7eec03e

Exports the ability to manually create a new cgroup filesystem struct, as well as the various mount modes. Cleans up documentation

hamiltont changed the title ~~Add more testing + memory stats from cgroups~~ WIP: Add more testing + memory stats from cgroups Feb 24, 2020

hamiltont mentioned this pull request Feb 26, 2020

Export unit substates #12

Open

anarcat mentioned this pull request May 10, 2022

memory usage stats are incorrect #46

Closed

oseiberts11 mentioned this pull request Nov 2, 2022

Fix incorrect process metric documentation. #65

Closed

SuperQ closed this Dec 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add more testing + memory stats from cgroups #10

WIP: Add more testing + memory stats from cgroups #10

hamiltont commented Feb 3, 2020 •

edited

Loading

povilasv commented Feb 6, 2020

hamiltont commented Feb 6, 2020

hamiltont commented Feb 6, 2020 •

edited

Loading

povilasv commented Feb 10, 2020

SuperQ commented Feb 10, 2020 •

edited

Loading

hamiltont commented Feb 10, 2020 via email •

edited

Loading

baryluk commented Feb 10, 2020

SuperQ commented Feb 10, 2020

baryluk commented Feb 10, 2020

povilasv Feb 13, 2020

povilasv Feb 13, 2020

povilasv Feb 13, 2020

povilasv Feb 14, 2020

povilasv Feb 14, 2020

povilasv Feb 14, 2020

povilasv Feb 14, 2020

povilasv Feb 14, 2020

SuperQ commented Feb 14, 2020

hamiltont commented Feb 24, 2020

hectorhuertas commented Apr 29, 2020

hamiltont commented Apr 29, 2020 via email

talyz commented Sep 13, 2021 •

edited

Loading

hamiltont commented Sep 13, 2021 via email

talyz commented Sep 14, 2021

anarcat commented May 10, 2022

SuperQ commented Jul 19, 2022

oseiberts11 commented Nov 2, 2022

	if err != nil {
	if err != nil && parseUnitType(unit) != "socket" {

WIP: Add more testing + memory stats from cgroups #10

WIP: Add more testing + memory stats from cgroups #10

Conversation

hamiltont commented Feb 3, 2020 • edited Loading

povilasv commented Feb 6, 2020

hamiltont commented Feb 6, 2020

hamiltont commented Feb 6, 2020 • edited Loading

povilasv commented Feb 10, 2020

SuperQ commented Feb 10, 2020 • edited Loading

hamiltont commented Feb 10, 2020 via email • edited Loading

baryluk commented Feb 10, 2020

SuperQ commented Feb 10, 2020

baryluk commented Feb 10, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperQ commented Feb 14, 2020

hamiltont commented Feb 24, 2020

hectorhuertas commented Apr 29, 2020

hamiltont commented Apr 29, 2020 via email

talyz commented Sep 13, 2021 • edited Loading

hamiltont commented Sep 13, 2021 via email

talyz commented Sep 14, 2021

anarcat commented May 10, 2022

SuperQ commented Jul 19, 2022

oseiberts11 commented Nov 2, 2022

hamiltont commented Feb 3, 2020 •

edited

Loading

hamiltont commented Feb 6, 2020 •

edited

Loading

SuperQ commented Feb 10, 2020 •

edited

Loading

hamiltont commented Feb 10, 2020 via email •

edited

Loading

talyz commented Sep 13, 2021 •

edited

Loading