opentelemetry-operations-collector repo rewrite #246

braydonk · 2025-03-07T03:25:29Z

This PR is a rewrite of the opentelemetry-operations-collector repo to the new version from the google-otel-poc branch. The new version has:

A tool called distrogen for generation and management of collector distributions
The original otelopscol binary buildable with ocb
The new Google-Built OpenTelemetry Collector
All the same original components as the original repo, modularized for use with OCB
An in-depth Makefile and developer tooling to make updating OTel versions for components as seamless as possible

ridwanmsharif · 2025-03-07T19:35:54Z

Interesting, do we need to make relevant kokoro config changes to make it work with this change?

braydonk · 2025-03-07T19:40:36Z

I think we should just get rid of that Kokoro setup, it is utterly unnecessary and has been for years.

bwplotka · 2025-03-11T11:49:20Z

Looks solid, nice!

LujieDuan

Nice work! Couple of small suggestions:

google-otel/Makefile

ridwanmsharif

note: I only reviewed distrogen. Most of my comments are noits and not blocking. Once responded to or addressed, I'll do another quick pass but things look pretty good now

ridwanmsharif · 2025-03-12T20:55:20Z

cmd/distrogen/distribution.go

+	"github.com/google/go-cmp/cmp"
+)
+
+var ErrNoDiff = errors.New("no differences found with previous generation")


Does this have to be exported?

ridwanmsharif · 2025-03-12T20:59:26Z

Makefile

+
+RUN_DISTROGEN=go run ./cmd/distrogen
+
+GEN_GOOGLE_OTEL=$(RUN_DISTROGEN) -registry ./registries/operations-collector-registry.yaml -spec ./specs/google-otel.yaml -custom_templates ./templates/google-otel


These are really cool make targets. Could you add a README.md in the distrogen tool with some instructions on what we expect each of these yaml files to contain?

Will do, but need a bit more time to write something nice.

ridwanmsharif · 2025-03-12T22:05:45Z

cmd/distrogen/yaml.go

+	var result T
+	err = yaml.Unmarshal(content, &result)
+	if err != nil {
+		return nil, wrapYamlErr(err, path)


Suggested change

return nil, wrapYamlErr(err, path)

if err != nil {

return nil, fmt.Errorf("failed to unmarshal file %s something: %w", path, originalErr)

}

return &result, nil

Why not just this? I don't see wrapYamlErr used anywhere else.

brain must have been turned off when I wrote this cause yeah idk why I did that. Fixed in 79e8314

ridwanmsharif · 2025-03-12T22:10:49Z

cmd/distrogen/main.go

+	return generateDistribution()
+}
+
+func generateSpec() error {


Neat idea. We don't have to recommend or document this option yet but I'm not sure if this is how I want to recommend folks build their GBOT spec. I imagine folks want a collector for a bunch of different configs/use cases and we don't want folks to build a different spec for each one. It is certainly a useful tool to find a minimal spec but this could confuse some people too.

Also, it doesn't do enough right? If my config uses an invalid receiver, or uses a forked component, I still would need to provide a registry.yaml file for it?

This feature is definitely half-baked. I forgot it was even here in the primary branch. I did it as a mockup for someone at KubeCon. I am okay with straight up removing the feature for now cause it's half-baked.

Yeah might be simpler to just remove for now

ridwanmsharif · 2025-03-12T22:13:42Z

cmd/distrogen/otel_config.go

+	if !ok {
+		return nil, fmt.Errorf("reading section %s: invalid section data", sectionName)
+	}
+	return mapKeys(section), nil


This is not entirely right is it? You can have receivers with the name prometheus/self-metrics and prometheus/app both referring to the same underlying receiver but the receiver is just prometheus.

cmd/distrogen/flags.go

ridwanmsharif · 2025-03-12T22:38:07Z

cmd/distrogen/registry.go

+
+// LoadAll will take a list of component names and load them
+// from the registry, attaching the appropriate version tag.
+func (rl RegistryList) LoadAll(names []string, version string, stableVersion string, contribVersion string) (RegistryComponents, RegistryLoadError) {


[nit] Load here is used in a different context than how its used when we called LoadRegistry. Lets fix one of the naming to something else

Load is such an equally good word for both things that I struggled to come up with something. Proposed something in 7442ce8

ridwanmsharif · 2025-03-12T22:44:37Z

cmd/distrogen/registry.go

+
+// RegistryComponents is a map of registry component names to component
+// details.
+type RegistryComponents map[string]*RegistryComponent


type RegistryList map[string]*RegistryComponent

Isn't this component effectively the same from one in line 70?

Yeah you're right, I didn't notice these types converge when I refactored a bunch of registry stuff. Removed and consolidated the methods in 79e8314

ridwanmsharif · 2025-03-12T22:47:44Z

cmd/distrogen/distribution.go

+
+	errs := make(RegistryLoadError)
+	var err RegistryLoadError
+	context.Receivers, err = registry.Receivers.LoadAll(spec.Components.Receivers, spec.OpenTelemetryVersion, spec.OpenTelemetryStableVersion, spec.OpenTelemetryContribVersion)


This translation from the RegistryList to RegistryComponents is confusing. They are the same no?

Seems like all this is doing is some cleanup of the gomod entry

In registry, the full list of all components exists. The context will only contain the actual configured components from the spec. Now that the type names are the same this is more confusing, not sure if this can either be done differently or if I should just add a comment clarifying.

cmd/distrogen/main.go

ridwanmsharif · 2025-03-14T03:59:25Z

cmd/distrogen/main.go

+	return generateDistribution()
+}
+
+func generateSpec() error {


Yeah might be simpler to just remove for now

cmd/distrogen/flags.go

ridwanmsharif · 2025-03-14T04:15:26Z

cmd/distrogen/registry.go

+	errs := make(RegistryLoadError)
+
+	for _, name := range names {
+		entry, err := rl.LoadComponent(name)


[nit] This is just a map lookup now, we can inline this when its called and get rid of this method.

Suggested change

entry, err := rl.LoadComponent(name)

entry, ok := rl[name]

if !ok {

errs[name] = ErrComponentNotFound

continue

}

I ended up refactoring this slightly in 055331f

The method is still there but it does things in a different order that makes more sense

ridwanmsharif · 2025-03-14T04:15:43Z

cmd/distrogen/registry.go

+}
+
+// LoadComponent will load a component from the registry.
+func (rl RegistryComponents) LoadComponent(name string) (*RegistryComponent, error) {


We don't need this I don't think, see other comment

ridwanmsharif

my comments don't block a merge fwiw. Feel free to resolve and address the rest

all distrogen changes LGTM. Lets get this in 🚀🚀🚀

LujieDuan

Reviewed other changes and LGTM! 🚀

internal/tools/cmd/otel_component_versions/x.txt

LICENSE

.github/header-checker-lint.yml

This PR is a rewrite of the opentelemetry-operations-collector repo to the new version from the `google-otel-poc` branch. The new version has: * A tool called `distrogen` for generations and managing collector distributions * The original otelopscol binary buildable with ocb * The new Google-Built OpenTelemetry Collector * All the same original components as the original repo, modularized for use with OCB * An in-depth Makefile and developer tooling to make updating OTel versions for components as seamless as possible

ridwanmsharif · 2025-03-18T16:29:46Z

FYI: I updated the branch protection rules on this repo so the Kokoro presubmits are non-blocking for merge to master.

braydonk force-pushed the google-otel-rewrite branch from cd1bfd6 to c3753a0 Compare March 7, 2025 19:29

LujieDuan reviewed Mar 11, 2025

View reviewed changes

google-otel/Makefile Outdated Show resolved Hide resolved

google-otel/Makefile Outdated Show resolved Hide resolved

google-otel/Makefile Outdated Show resolved Hide resolved

ridwanmsharif reviewed Mar 12, 2025

View reviewed changes

ridwanmsharif self-requested a review March 14, 2025 04:09

ridwanmsharif reviewed Mar 14, 2025

View reviewed changes

ridwanmsharif approved these changes Mar 14, 2025

View reviewed changes

LujieDuan approved these changes Mar 14, 2025

View reviewed changes

internal/tools/cmd/otel_component_versions/x.txt Outdated Show resolved Hide resolved

LICENSE Show resolved Hide resolved

.github/header-checker-lint.yml Show resolved Hide resolved

braydonk and others added 16 commits March 14, 2025 20:02

fix gitignore so otelopscol isn't ignored

498dc8d

change to use go command directly for collector build

3568c1b

add powershell build script template

5d7ac4a

generate powershell buildscript template

664a56e

sync components from master to this branch

34805c0

add license headers to metadata files

5a2c3e7

add missing components to both distros

492aaa4

address comments from Lujie

520fa34

address low-hanging fruit from Ridwan

757e321

rename load methods

24535c5

add golden tests for distrogen

055331f

Add oauth2authextension from upstream with no code changes (#249)

dc65657

restore Github Action presubmits

ce171de

fixing a bunch of processor test failures I thought were fixed

ee880fb

fix all test code and make tests run properly on gpu receivers

65c09be

braydonk force-pushed the google-otel-rewrite branch from 59043c8 to 65c09be Compare March 14, 2025 20:03

braydonk added 2 commits March 14, 2025 20:21

add license file

58999a6

remove intermediate test file

2452a76

LujieDuan approved these changes Mar 14, 2025

View reviewed changes

change otelcol google version in docs

c8076eb

braydonk merged commit cb12823 into master Mar 18, 2025
3 of 6 checks passed


		RUN_DISTROGEN=go run ./cmd/distrogen

		GEN_GOOGLE_OTEL=$(RUN_DISTROGEN) -registry ./registries/operations-collector-registry.yaml -spec ./specs/google-otel.yaml -custom_templates ./templates/google-otel

-		entry, err := rl.LoadComponent(name)
+		entry, ok := rl[name]
+        if !ok {
+            errs[name] = ErrComponentNotFound
+            continue
+        }

opentelemetry-operations-collector repo rewrite #246

opentelemetry-operations-collector repo rewrite #246

Uh oh!

Conversation

braydonk commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ridwanmsharif commented Mar 7, 2025

Uh oh!

braydonk commented Mar 7, 2025

Uh oh!

bwplotka commented Mar 11, 2025

Uh oh!

LujieDuan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ridwanmsharif left a comment

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

ridwanmsharif left a comment

Choose a reason for hiding this comment

Uh oh!

LujieDuan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ridwanmsharif commented Mar 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

braydonk commented Mar 7, 2025 •

edited

Loading