Config validate by ogaca-dd · Pull Request #859 · open-telemetry/opentelemetry-ebpf-profiler

ogaca-dd · 2025-10-08T14:14:31Z

This PR automatically validates the Configuration:

Move collector.Config to its own package config to avoid dependency cycle.
Embeds the collector.Config into the controller.Config
Move controller.Config.Validate content to config.Config.Validate

It is a replacement for #849.

Note: Reviewing commit by commit is recommended.

Embeds config.Config into controller.Config

florianl · 2025-10-08T14:32:15Z

+				"should be in the range [0..1]. 0 disables off-cpu profiling")
+	}
+
+	if !cfg.NoKernelVersionCheck {


The following part is mixing configuration validation with system validation. This should be avoided.

I have just moved the existing code.

maybe it's time to separate it.

System checks should maybe stay in internal/controller while configuration checks should be close to collector/config.

florianl · 2025-10-08T14:39:39Z

+			if cfg.VerboseMode {
+				minMajor, minMinor = 5, 2
+			} else {
+				minMajor, minMinor = 4, 19


This check is incorrect. 4.19 is no longer supported, as the project moved on and the instruction limit (4096) is no longer enforced on eBPF programs. As an example - unwind_native currently uses 7014 instructions on amd64.
The bump of the kernel version is therefore not related to the changes around cfg.VerboseMode, but it enabled the changes around cfg.VerboseMode.

The minimal supported kernel version (for x86) should align with https://github.com/open-telemetry/opentelemetry-ebpf-profiler?tab=readme-ov-file#supported-linux-kernel-version and not make a difference based on cfg.VerboseMode.

ogaca-dd · 2025-10-08T15:08:33Z

How to fix CI / Check licenses of dependencies (pull_request)?

Running make legal locally removes a lot of files.

florianl · 2025-10-08T15:16:04Z

+		return fmt.Errorf("invalid sampling frequency: %d", cfg.SamplesPerSecond)
+	}
+
+	if cfg.MapScaleFactor > 8 {


Suggested change

if cfg.MapScaleFactor > 8 {

if cfg.MapScaleFactor > MaxArgMapScaleFactor {

florianl · 2025-10-08T15:18:19Z

+				"should be in the range [0..1]. 0 disables off-cpu profiling")
+	}
+
+	if !cfg.NoKernelVersionCheck {


maybe it's time to separate it.

florianl · 2025-10-08T15:21:41Z

+	BPFVerifierLogLevel    uint          `mapstructure:"bpf_verifier_log_level"`
+	NoKernelVersionCheck   bool          `mapstructure:"no_kernel_version_check"`
+	MaxGRPCRetries         uint32        `mapstructure:"max_grpc_retries"`
+	MaxRPCMsgSize          int           `mapstructure:"max_rpc_msg_size"`


Should there be a check on MaxRPCMsgSize? As it is of type int, it could also be negative.

florianl · 2025-10-08T15:28:19Z

How to fix CI / Check licenses of dependencies (pull_request)?

Running make legal locally removes a lot of files.

Running make clean; make legal will add the missing legal files. Here, this is:

?? LICENSES/github.com/go-viper/
?? LICENSES/github.com/gobwas/
?? LICENSES/github.com/knadh/
?? LICENSES/github.com/mitchellh/
?? LICENSES/go.opentelemetry.io/collector/confmap/
?? LICENSES/go.uber.org/zap/
?? LICENSES/go.yaml.in/

In this process, no file was removed.

dmathieu · 2025-10-08T18:00:17Z

This does way more than just validate config. Could you split it?

ogaca-dd · 2025-10-09T10:01:28Z

@florianl
After running make clean && make legal I have the following files deleted

Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        deleted:    LICENSES/github.com/cilium/ebpf/LICENSE
        deleted:    LICENSES/github.com/elastic/go-freelru/LICENSE
        deleted:    LICENSES/github.com/elastic/go-freelru/NOTICE
        deleted:    LICENSES/github.com/elastic/go-perf/LICENSE
        deleted:    LICENSES/github.com/gogo/protobuf/LICENSE
        deleted:    LICENSES/github.com/google/uuid/LICENSE
        deleted:    LICENSES/github.com/hashicorp/go-version/.github/dependabot.yml
        deleted:    LICENSES/github.com/hashicorp/go-version/.github/workflows/go-tests.yml
        deleted:    LICENSES/github.com/hashicorp/go-version/CHANGELOG.md
        deleted:    LICENSES/github.com/hashicorp/go-version/LICENSE
        deleted:    LICENSES/github.com/hashicorp/go-version/README.md
        deleted:    LICENSES/github.com/hashicorp/go-version/constraint.go
        deleted:    LICENSES/github.com/hashicorp/go-version/constraint_test.go
        deleted:    LICENSES/github.com/hashicorp/go-version/go.mod
        deleted:    LICENSES/github.com/hashicorp/go-version/version.go
        deleted:    LICENSES/github.com/hashicorp/go-version/version_collection.go
        deleted:    LICENSES/github.com/hashicorp/go-version/version_collection_test.go
        deleted:    LICENSES/github.com/hashicorp/go-version/version_test.go
        deleted:    LICENSES/github.com/josharian/native/license
        deleted:    LICENSES/github.com/json-iterator/go/LICENSE
        deleted:    LICENSES/github.com/klauspost/cpuid/v2/LICENSE
        deleted:    LICENSES/github.com/mdlayher/kobject/LICENSE.md
        deleted:    LICENSES/github.com/mdlayher/netlink/LICENSE.md
        deleted:    LICENSES/github.com/mdlayher/socket/LICENSE.md
        deleted:    LICENSES/github.com/minio/sha256-simd/LICENSE
        deleted:    LICENSES/github.com/modern-go/concurrent/LICENSE
        deleted:    LICENSES/github.com/modern-go/reflect2/LICENSE
        deleted:    LICENSES/github.com/peterbourgon/ff/v3/LICENSE
        deleted:    LICENSES/github.com/sirupsen/logrus/LICENSE
        deleted:    LICENSES/github.com/zeebo/xxh3/LICENSE
        deleted:    LICENSES/go.opentelemetry.io/collector/consumer/LICENSE
        deleted:    LICENSES/go.opentelemetry.io/collector/consumer/xconsumer/LICENSE
        deleted:    LICENSES/go.opentelemetry.io/collector/featuregate/LICENSE
        deleted:    LICENSES/go.opentelemetry.io/collector/pdata/LICENSE
        deleted:    LICENSES/go.opentelemetry.io/collector/pdata/pprofile/LICENSE
        deleted:    LICENSES/go.opentelemetry.io/ebpf-profiler/LICENSE
        deleted:    LICENSES/go.opentelemetry.io/otel/LICENSE
        deleted:    LICENSES/go.opentelemetry.io/otel/metric/LICENSE
        deleted:    LICENSES/go.uber.org/multierr/LICENSE.txt
        deleted:    LICENSES/golang.org/x/arch/LICENSE
        deleted:    LICENSES/golang.org/x/exp/constraints/LICENSE
        deleted:    LICENSES/golang.org/x/net/LICENSE
        deleted:    LICENSES/golang.org/x/sync/errgroup/LICENSE
        deleted:    LICENSES/golang.org/x/sys/unix/LICENSE
        deleted:    LICENSES/golang.org/x/text/LICENSE
        deleted:    LICENSES/google.golang.org/genproto/googleapis/rpc/status/LICENSE
        deleted:    LICENSES/google.golang.org/grpc/LICENSE
        deleted:    LICENSES/google.golang.org/grpc/NOTICE.txt
        deleted:    LICENSES/google.golang.org/protobuf/LICENSE

I ran the command on Ubuntu ARM64. Any ideas?

ogaca-dd · 2025-10-09T10:10:36Z

@dmathieu

This does way more than just validate config. Could you split it?

This PR has the minimal changes to validate the config using this recommend solution.
As there were quite a lot of back and forth (This is the third version) can we make agree first on this PR about all the changes required and then I'll split this PR into small PR?

dmathieu · 2025-10-09T12:33:33Z

This PR also includes a refactoring of the config, which is a nice thing. But that's what I mean should be its own PR.
Making small PRs is actually intended to reduce back and forth, as it helps review the code in smaller chunks rather than large ones.

ogaca-dd · 2025-10-16T09:32:18Z

Close in favor of:

ogaca-dd added 3 commits October 8, 2025 15:35

Move collector.Config to its own package.

0ff6a6e

Embeds config.Config into controller.Config

Move controller.Config.Validate content to config.Config.Validate

b9ce344

Add a test to validate configuration

064ec72

florianl reviewed Oct 8, 2025

View reviewed changes

ogaca-dd mentioned this pull request Oct 8, 2025

Validate the controller config. #849

Closed

florianl reviewed Oct 8, 2025

View reviewed changes

ogaca-dd marked this pull request as ready for review October 8, 2025 15:08

ogaca-dd requested review from a team as code owners October 8, 2025 15:08

florianl reviewed Oct 8, 2025

View reviewed changes

This was referenced Oct 10, 2025

Move collector.Config to its own package. #864

Merged

Embed config.Config to controller.Config #867

Merged

Validate MaxRPCMsgSize in controller config #868

Merged

dmathieu closed this Oct 16, 2025

florianl mentioned this pull request Oct 29, 2025

Move config validate #897

Merged

	if cfg.MapScaleFactor > 8 {
	if cfg.MapScaleFactor > MaxArgMapScaleFactor {

Conversation

ogaca-dd commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

florianl Oct 8, 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!

ogaca-dd commented Oct 8, 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!

florianl commented Oct 8, 2025

Uh oh!

dmathieu commented Oct 8, 2025

Uh oh!

ogaca-dd commented Oct 9, 2025

Uh oh!

ogaca-dd commented Oct 9, 2025

Uh oh!

dmathieu commented Oct 9, 2025

Uh oh!

ogaca-dd commented Oct 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ogaca-dd commented Oct 8, 2025 •

edited

Loading

florianl Oct 8, 2025 •

edited

Loading