Skip to content

Centralize daemon metrics#49165

Merged
thaJeztah merged 5 commits intomoby:masterfrom
vvoland:image-metrics-move
Jan 7, 2025
Merged

Centralize daemon metrics#49165
thaJeztah merged 5 commits intomoby:masterfrom
vvoland:image-metrics-move

Conversation

@vvoland
Copy link
Copy Markdown
Contributor

@vvoland vvoland commented Dec 23, 2024 •
edited
Loading

Centralize daemon metrics in an internal/metrics package.

@vvoland vvoland added status/2-code-review area/images Image Service kind/refactor PR's that refactor, or clean-up code containerd-integration Issues and PRs related to containerd integration labels Dec 23, 2024
@vvoland vvoland added this to the 28.0.0 milestone Dec 23, 2024
@vvoland vvoland self-assigned this Dec 23, 2024
@vvoland vvoland requested a review from tonistiigi as a code owner December 23, 2024 12:00
@vvoland vvoland changed the title daemon: Move image metrics to daemon images: Move image metrics to daemon Dec 23, 2024
thaJeztah
thaJeztah reviewed Dec 23, 2024
View reviewed changes
Comment thread daemon/images/metrics.go
thaJeztah
thaJeztah reviewed Dec 23, 2024
View reviewed changes
Comment thread daemon/containerd/fake_service_test.go Outdated
Comment on lines +294 to +297
var testImageActions = sync.OnceValue(func() metrics.LabeledTimer {
ns := metrics.NewNamespace("test", "containerd", nil)
return ns.NewLabeledTimer("image_action", "Image actions", "action")
})
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started to wonder; how much sense would it make to move all those timers to a metrics package, that would have some sync.OnceValue to get them? (instead of making these a field in the daemon).

Not sure if it's better, or if it would be worse. Mostly looking that some of the initialization is a bit sprinkled throughout, which means that some namespaces are registered multiple times in different locations, e.g. there's still one in daemon/events;

moby/daemon/events/metrics.go

Lines 10 to 15 in a72026a

func init() {
ns := metrics.NewNamespace("engine", "daemon", nil)
eventsCounter = ns.NewCounter("events", "The number of events logged")
eventSubscribers = ns.NewGauge("events_subscribers", "The number of current subscribers to events", metrics.Total)
metrics.Register(ns)
}

But I guess that won't work for this specific case if we want separate counters for testing

@vvoland vvoland changed the title images: Move image metrics to daemon Centralize daemon metrics Dec 23, 2024
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Dec 23, 2024

Looks like some file may need a build-tag;

#38 0.510 + go build -mod=vendor -modfile=vendor.mod -o /tmp/bundles/binary-daemon/dockerd.exe -tags ' osusergo static_build ' -ldflags '-w -X "github.com/docker/docker/dockerversion.Version=pr-49165" -X "github.com/docker/docker/dockerversion.GitCommit=1d17fa1b4f3780043bae865643e9c68afa83e17d" -X "github.com/docker/docker/dockerversion.BuildTime=2024-12-23T15:34:45.000000000+00:00" -X "github.com/docker/docker/dockerversion.PlatformName=Moby Engine - Nightly" -X "github.com/docker/docker/dockerversion.ProductName=moby-bin" -X "github.com/docker/docker/dockerversion.DefaultProductLicense="  -extldflags -static ' -gcflags= github.com/docker/docker/cmd/dockerd
#38 23.99 # github.com/docker/docker/internal/metrics
#38 23.99 internal/metrics/plugin.go:110:7: undefined: unix.Unlink

@vvoland vvoland force-pushed the image-metrics-move branch 2 times, most recently from b89a535 to 4777aae Compare December 23, 2024 16:02
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Dec 28, 2024

One more linter failure 😅

daemon/daemon.go:126:2: field `metricsPluginListener` is unused (unused)
	metricsPluginListener net.Listener
	^

@vvoland vvoland force-pushed the image-metrics-move branch from 4777aae to 01d451b Compare January 7, 2025 08:32
laurazard
laurazard approved these changes Jan 7, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@laurazard laurazard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! This is much better.

thaJeztah
thaJeztah approved these changes Jan 7, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

found one find/replace issue; not a blocker for sure, but in case you want to fix it up ❤️

Comment thread daemon/container_operations.go Outdated
Comment thread internal/metrics/metrics.go
@@ -0,0 +1,130 @@
package metrics
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a follow-up; depending where this all ends up in (i.e. if it's also included in packages that are imported externally), we could even consider having a build-tag to disable metrics at compile time.

vvoland added 5 commits January 7, 2025 14:13
@vvoland
Alias github.com/docker/go-metrics imports
048fece 
Next commits will introduce a new internal `metrics` package, so alias
the "external" import to avoid confusion.

Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland
daemon/metrics: Move out to internal/metrics
51c2689 
Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland
daemon/images: Move ImageActions to metrics
438f580 
Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland
daemon: Move direct usages of go-metrics to internal/metrics
90e2afd 
Add wrapper for StartTimer inside the internal package

Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland
daemon/events: Move metrics to internal/metrics
0f7a432 
Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland vvoland force-pushed the image-metrics-move branch from 01d451b to 0f7a432 Compare January 7, 2025 13:13
@thaJeztah thaJeztah merged commit 72829b7 into moby:master Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

@laurazard laurazard laurazard approved these changes

@tonistiigi tonistiigi Awaiting requested review from tonistiigi tonistiigi is a code owner

Assignees

@vvoland vvoland

Labels

area/daemon Core Engine area/images Image Service area/metrics containerd-integration Issues and PRs related to containerd integration kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Milestone

28.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@vvoland @thaJeztah @laurazard