Add neco-server-exporter to expose custom node-local metrices #1753

yokaze · 2025-10-22T05:38:32Z

Add neco-server-exporter to expose custom node-local metrices
- Focus on performance digit not exposed from node-exporter nor cAdvisor
- Extensible structure
  - It can collect information from multiple aspects of a server
  - BPF performance monitoring as the first plugin
    - Assume Linux 6.6+ and Cilium 1.16+ (TCX mode) to collect extended information

Signed-off-by: Daichi Sakaue [email protected]

Signed-off-by: Daichi Sakaue <[email protected]>

yokaze · 2025-10-30T06:31:47Z

NOTE: merge this PR after preparing ghcr.io

Signed-off-by: Daichi Sakaue <[email protected]>

chez-shanpu · 2025-11-04T02:09:19Z

neco-server-exporter/pkg/exporter/exporter.go

+	return nil
+}
+
+func (e *Exporter) Run(ctx context.Context) error {


It seems that Run() never return an error.

I think it's better to leave it as is, because it states the Run function may return error in future development.
Without it, someone may confuse how to treat errors happen in it.

That makes sense. How about writing it in a comment to left what you replied?
For example...

// Run starts the metric collection loop. // The error return is reserved for future fatal error conditions. // Currently always returns nil except when context is cancelled. func (e *Exporter) Run(ctx context.Context) error {

Sounds good, I will add it.

chez-shanpu · 2025-11-04T02:30:16Z

neco-server-exporter/pkg/exporter/exporter.go

+	}
+}
+
+func (e *Exporter) AddCollector(c Collector) error {


suggestion: How about making collectors a constructor parameter instead of using AddCollector(). This would remove running and make the API simpler.

Cool idea, thank you!

chez-shanpu · 2025-11-04T02:44:43Z

neco-server-exporter/pkg/exporter/exporter.go

+	if err := http.ListenAndServe("0.0.0.0:8080", nil); err != nil {
+		e.log.Error("metrics server stopped", slog.Any("error", err))
+		return err
+	}


This HTTP server doesn't stop when ctx is cancelled. How about using http.Server.Shutdown() when ctx is done for graceful shutdown?

chez-shanpu · 2025-11-04T02:50:10Z

neco-server-exporter/pkg/collector/bpf/collector.go

+	"github.com/cilium/cilium/pkg/client"
+	"github.com/cilium/ebpf"
+	"github.com/cybozu/neco-containers/neco-server-exporter/pkg/exporter"


Suggested change

"github.com/cilium/cilium/pkg/client"

"github.com/cilium/ebpf"

"github.com/cybozu/neco-containers/neco-server-exporter/pkg/exporter"

"github.com/cilium/cilium/pkg/client"

"github.com/cilium/ebpf"

"github.com/cybozu/neco-containers/neco-server-exporter/pkg/exporter"

Missing blank line between external and internal imports.

This point is new to me, because the proposed style is not currently adopted by our team.

Codes dividing external and internal imports

https://github.com/cybozu/neco-containers/blob/main/cep-checker/main.go

https://github.com/cybozu-go/tenet/blob/main/cmd/tenet-controller/main.go

Codes not doing so

https://github.com/cybozu/neco-containers/blob/main/admission/cmd/run.go

https://github.com/cybozu-go/coil/blob/main/v2/cmd/coil-installer/sub/root.go

My feeling is neutral for this and it's ok to accept it. However, it's better to automate.
I'm going to install goimports in tools.mk and check the style in make check-generate.

chez-shanpu · 2025-11-04T02:50:29Z

neco-server-exporter/main.go

+	"github.com/cybozu/neco-containers/neco-server-exporter/pkg/collector/bpf"
+	"github.com/cybozu/neco-containers/neco-server-exporter/pkg/exporter"
+	"github.com/spf13/cobra"
+	ctrl "sigs.k8s.io/controller-runtime"


chez-shanpu · 2025-11-04T08:12:31Z

neco-server-exporter/e2e/Makefile

+uninstall:
+	$(KUBECTL) delete -f testdata/namespace.yaml
+	-docker image rm $(IMAGE_TAG)
+	-docker image prune -f


Isn't it enough docker image rm $(IMAGE_TAG)?
image prune seems little dangerous to me.

It's consistent with other scripts.
Do you want all of them to be fixed?

$ git grep -F -- '-docker image prune -f' cep-checker/e2e/Makefile:43: -docker image prune -f neco-server-exporter/e2e/Makefile:57: -docker image prune -f squid-exporter/e2e/Makefile:34: -docker image prune -f

I think other ones could be fixed as well, but for now, I think only this one is fine.

Please let me clarify my implication, I think it's very important to choose one from the following options.

Fix all-at-once.

Leave it as negligible.

If we fix the problem partially, the remaining ones will be copy-pasted sometime and we need to point out the same problem again and again. That's not productive.
I would like to ask that the problem is worth fixing for paying the all-at-once cost.

I see. What do you think about image prune?
If you also think it's a problem, let's fix them.

To my opinion it looks a little dangerous too.
It's not intuitive that make uninstall prunes all the unrelated images.

My preference is to remove automatic image prune from the entire repository.

chez-shanpu · 2025-11-04T08:15:08Z

neco-server-exporter/e2e/utils_test.go

+
+// Please uncomment when needed
+
+// func kubectl(input []byte, args ...string) ([]byte, []byte, error) {
+// 	return runCommand(kubectlPath, input, args...)
+// }


nit: I think we can remove these lines (I think we can add it when it's needed).

I think it's better to leave it here, because the function will very likely be needed.
Otherwise, the one who wants kubectl will need to grep neco repositories all-around to get an appropriate version.

ok, why don't you call kubectl() from kubectlSafe()?

This is a tiny optimization to save call-stack memory that's not-so-effective (~10ns/32Bytes).
I'll use kubectl() from kubectlSafe(), that's more rigid and readable.

chez-shanpu · 2025-11-04T08:22:42Z

neco-server-exporter/pkg/collector/bpf/helper.go

+	ret := make(map[ebpf.ProgramID]TCXMetadata)
+	for it.Next() {
+		li := it.Take()
+		defer li.Close()


It should be close end of for or extract it as a function.

I think it's not a major problem, but will fix the implementation to close the handle per-program to avoid consuming thousands of file descriptors caused by a large number of BPF programs.

chez-shanpu · 2025-11-04T08:27:25Z

neco-server-exporter/pkg/exporter/types.go

+
+type Collector interface {
+	// Metrics names will be "neco_server_<SectionName>_<MetricsName>{MetricsLabels}".
+	SectionName() string


nit: I think just Name() would be fine too, since it's clear that it's the collector's(section?) name.

I prefer to use SectionName, because it flavors like something special is there.
Name looks too concise to find out it's used as a part of metrics names.
Someone may think it's ok to set arbitrary text here and write as "BPF Performance Exporter".

BTW how about MetricsGroupName()?

MetricsGroupName() is fine. Also MetricsPrefix is clear to me in this case.

OK I'm going to use MetricsPrefix.
Thanks for the suggestion 🙇

chez-shanpu · 2025-11-04T08:29:39Z

neco-server-exporter/go.mod

@@ -0,0 +1,120 @@
+module github.com/cybozu/neco-containers/neco-server-exporter
+
+go 1.24.5


nit: 1.24.5 has some vulnerabilities. I think the latest would be good.

The version should exactly match with the workflow:
https://github.com/cybozu/neco-containers/blob/main/.github/workflows/main.yaml#L281
https://github.com/cybozu/neco-containers/actions/runs/19063539929/job/54448614430

Do you think it's ok to postpone to regular update, or better to update all for now?

$ git grep -F 1.24.5 .github/workflows/main.yaml:285: go-version: "1.24.5" admission/go.mod:3:go 1.24.5 bmc-log-collector/go.mod:3:go 1.24.5 cep-checker/go.mod:3:go 1.24.5 envoy/go.mod:3:go 1.24.5 neco-server-exporter/go.mod:3:go 1.24.5 squid-exporter/go.mod:3:go 1.24.5 tcp-keepalive/go.mod:3:go 1.24.5

For now, I think it's enough to just update this one.

It's impossible to fix neco-server-exporter only, due to repository structure.
We must choose all-at-once or regular update for this problem.

Signed-off-by: Daichi Sakaue <[email protected]>

yokaze self-assigned this Oct 22, 2025

yokaze force-pushed the neconet-exporter branch 8 times, most recently from 4574776 to 65f5f30 Compare October 29, 2025 04:13

yokaze force-pushed the neconet-exporter branch 3 times, most recently from 4e7564b to 57d1bc0 Compare October 30, 2025 01:11

Add neco-server-exporter to expose custom node-local metrices

573b19e

Signed-off-by: Daichi Sakaue <[email protected]>

yokaze force-pushed the neconet-exporter branch from 57d1bc0 to 573b19e Compare October 30, 2025 06:18

yokaze marked this pull request as ready for review October 30, 2025 06:29

yokaze requested a review from chez-shanpu October 30, 2025 06:31

yokaze force-pushed the neconet-exporter branch 2 times, most recently from aca806e to 4f2916e Compare November 4, 2025 07:34

yokaze added 3 commits November 4, 2025 16:48

neco-server-exporter: refactor

8ce8367

Signed-off-by: Daichi Sakaue <[email protected]>

neco-server-exporter: enforce section name

2a5c7c5

Signed-off-by: Daichi Sakaue <[email protected]>

neco-server-exporter: add collector metrics

4c96d64

Signed-off-by: Daichi Sakaue <[email protected]>

yokaze force-pushed the neconet-exporter branch from 4f2916e to 4c96d64 Compare November 4, 2025 07:50

chez-shanpu reviewed Nov 4, 2025

View reviewed changes

yokaze force-pushed the neconet-exporter branch 5 times, most recently from 427e2d0 to 8e2eb90 Compare November 5, 2025 07:57

wip: Reflect comments

bde9ccf

Signed-off-by: Daichi Sakaue <[email protected]>

yokaze force-pushed the neconet-exporter branch from 8e2eb90 to bde9ccf Compare November 5, 2025 09:37

		@@ -0,0 +1,120 @@
		module github.com/cybozu/neco-containers/neco-server-exporter

		go 1.24.5

Add neco-server-exporter to expose custom node-local metrices #1753

Are you sure you want to change the base?

Add neco-server-exporter to expose custom node-local metrices #1753

Conversation

yokaze commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yokaze commented Oct 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yokaze Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yokaze commented Oct 22, 2025 •

edited

Loading

yokaze Nov 4, 2025 •

edited

Loading