Restructure repository for OTel version update#286
Conversation
The structure for components in this repo had a fundamental issue. Once we introduced components that were specific to Google-Built OpenTelemetry Collector, we had the issue of otelopscol components targeting an older version of OpenTelemetry. We don't want to force all components to be on the same OTel version; when targeting different distributions, the components need to be at whichever version that distribution is on. This PR restructures the component directories to separate them into two separate directories, one for each distribution the component could be targeting. The Makefile structure is adjusted around this fact. The new process for upating OTel components for a distribution is to go into the Makefile for that distribution's components folder and update the version variables, then in the main directory call the associated target to update those components. This PR also does the OTel version updates for Google-Built OpenTelemetry Collector to prove that the restructure worked effectively. Updates were needed to the client auth extension based on: open-telemetry/opentelemetry-collector-contrib#38451 open-telemetry/opentelemetry-collector-contrib#38601
braydonk
requested review from
LujieDuan,
XuechunHou,
jinghan-ma and
ridwanmsharif
Adding distrogen spec querying allows for us to control our component updates based on the otel versions in the spec file as a single source of truth.
jinghan-ma
left a comment
jinghan-ma
left a comment
There was a problem hiding this comment.
It would also be nice to add a short section in the readme to document the process for changing the target version (e.g. change version in .yaml spec, run update-google-otel-components, etc.)
| GOOGLE_OTEL_SPEC_QUERY = go run ./cmd/distrogen -spec specs/google-built-opentelemetry-collector.yaml -query | ||
| .PHONY: update-google-otel-components | ||
| update-google-otel-components: export OTEL_VERSION := v$(shell $(GOOGLE_OTEL_SPEC_QUERY) opentelemetry_version) | ||
| update-google-otel-components: export OTEL_CONTRIB_VERSION := v$(shell $(GOOGLE_OTEL_SPEC_QUERY) opentelemetry_contrib_version) | ||
| update-google-otel-components: install-tools | ||
| cd components/google-built-opentelemetry-collector && PATH="$(TOOLS_DIR):${PATH}" $(MAKE) update-components | ||
|
|
||
| OTELOPSCOL_SPEC_QUERY = go run ./cmd/distrogen -spec specs/otelopscol.yaml -query | ||
| .PHONY: update-otelopscol-components | ||
| update-otelopscol-components: export OTEL_VERSION := v$(shell $(OTELOPSCOL_SPEC_QUERY) opentelemetry_version) | ||
| update-otelopscol-components: export OTEL_CONTRIB_VERSION := v$(shell $(OTELOPSCOL_SPEC_QUERY) opentelemetry_contrib_version) | ||
| update-otelopscol-components: install-tools | ||
| cd components/otelopscol && PATH="$(TOOLS_DIR):${PATH}" $(MAKE) update-components |
There was a problem hiding this comment.
Very nit picky, but it takes a bit of reading to realize that these two do the exact same thing, just on different directories. If we expect this to remain true in the future, maybe combining the definitions would make it our intentions more clear?
Something like:
.PHONY: update-google-otel-components update-otelopscol-components
update-google-otel-components: SPEC_FILE := specs/google-built-opentelemetry-collector.yaml
update-google-otel-components: COMPONENT_DIR := components/google-built-opentelemetry-collector
update-otelopscol-components: SPEC_FILE := specs/otelopscol.yaml
update-otelopscol-components: COMPONENT_DIR := components/otelopscol
update-google-otel-components update-otelopscol-components: DISTROGEN_QUERY := go run ./cmd/distrogen -spec $(SPEC_FILE) -query
update-google-otel-components update-otelopscol-components: export OTEL_VERSION := v$(shell $(DISTROGEN_QUERY) opentelemetry_version)
update-google-otel-components update-otelopscol-components: export OTEL_CONTRIB_VERSION := v$(shell $(DISTROGEN_QUERY) opentelemetry_contrib_version)
update-google-otel-components update-otelopscol-components: install-tools
cd $(COMPONENT_DIR) && PATH="$(TOOLS_DIR):${PATH}" $(MAKE) update-components
I don't have strong opinions either way.
| } | ||
|
|
||
| // Don't include .tools directory in comparison. | ||
| if strings.Contains(path, ".tools") { |
There was a problem hiding this comment.
Another super nitpick.
Since we already exclude all directories in the previous if statement. I think checking for /.tools/ in the path should still exclude contents in the .tools directory while excluding false positives like files/directories that contains the .tools substring like google.otel.tools (although it's super unlikely)
There was a problem hiding this comment.
Excluding all directories just excludes the individual entry for that directory; filepath.WalkDir will still recursively walk the subdirectory even if we ignore the specific directory entry it presented us. So the check is still necessary.
Applied suggestion to make the check stricter in 8dc4905
| .PHONY: update-otel-components | ||
| update-components: export OTEL_VERSION := $(OTEL_VERSION) | ||
| update-components: export OTEL_CONTRIB_VERSION := $(OTEL_CONTRIB_VERSION) | ||
| update-components: update-deps tidy-components update-mdatagen generate-components |
There was a problem hiding this comment.
Are these intended to be update-components or update-otel-components?
There was a problem hiding this comment.
Named it update-components here because calling them otel components was redundant (all components in a component dir are otel)
braydonk
force-pushed
the
restructure-modules
branch
from
1889a70 to
8dc4905
Compare
Added instructions in 397bae8 |
4 participants
The structure for components in this repo had a fundamental issue. Once we introduced components that were specific to Google-Built OpenTelemetry Collector, we had the issue of otelopscol components targeting an older version of OpenTelemetry. We don't want to force all components to be on the same OTel version; when targeting different distributions, the components need to be at whichever version that distribution is on.
This PR restructures the component directories to separate them into two separate directories, one for each distribution the component could be targeting. The Makefile structure is adjusted around this fact. The new process for upating OTel components for a distribution is
to go into the Makefile for that distribution's components folder and update the version variablesupdate the distrogen spec with the new intended OTel versions and then in the main directory call the associated target to update those components.This PR also does the OTel version updates for Google-Built OpenTelemetry Collector to prove that the restructure worked effectively.
Updates were needed to the client auth extension based on: open-telemetry/opentelemetry-collector-contrib#38451 open-telemetry/opentelemetry-collector-contrib#38601