Refactor component and factory interface definitions #683

tigrannajaryan · 2020-03-23T18:55:39Z

As we are preparing for Beta release we want to cleanup publicly exported
types and interfaces and do all necessary refactoring and breaking changes
now, before the Beta release. We will have a lot less leeway for breaking
changes after the Beta release.

Component related type declarations are now all in the component package.
This makes it possible for the interfaces to reference each other. This
was were very restricted earlier because component interfaces were in 5
different packages and many proposals were impossible to implement because
they would result in circular dependencies between packages.

(An example upcoming new capability that is enabled by this refactoring
is for components to query the Host for other components and for factories).

List of changes in this commit:

Move all factory interfaces and component interfaces to component package.
Rename old factories and components interfaces to use "Old" suffix for clarity.
Eliminate forced checks that components implement factories. This is already
enforced by the compiler when the factory is added to the Defaults() and
was unnecessary code.
Eliminated some unnecessary codes (removed overall over 200 lines).
Run go mod tidy on testbed.

Warning: this is a breaking change to publicly exported types and function
signatures. We announced that a breaking change is comming. Once we agree
to merge this commit we will need to announce the exact list of changes
and guide component authors to modify their components accordingly.

Future changes:

Once all components are migrated to the new internal representation,
delete all "Old"-suffixed definitions. This will likely be done while
we are still in Beta phase, before the Stable release.

codecov-io · 2020-03-23T19:34:47Z

Codecov Report

Merging #683 into master will decrease coverage by 0.06%.
The diff coverage is 77.58%.

@@            Coverage Diff             @@
##           master     #683      +/-   ##
==========================================
- Coverage   74.36%   74.29%   -0.07%     
==========================================
  Files         162      159       -3     
  Lines       11385    11391       +6     
==========================================
- Hits         8466     8463       -3     
- Misses       2500     2508       +8     
- Partials      419      420       +1

Impacted Files	Coverage Δ
exporter/exportertest/sink_exporter.go	`69.23% <ø> (ø)`
extension/extensiontest/mock_host.go	`84.61% <ø> (ø)`
...nsion/healthcheckextension/healthcheckextension.go	`92.85% <ø> (ø)`
extension/pprofextension/pprofextension.go	`64.00% <ø> (ø)`
extension/zpagesextension/zpagesextension.go	`89.47% <ø> (ø)`
receiver/opencensusreceiver/opencensus.go	`90.09% <ø> (ø)`
receiver/otlpreceiver/otlp.go	`86.48% <ø> (ø)`
receiver/prometheusreceiver/metrics_receiver.go	`77.77% <ø> (ø)`
receiver/vmmetricsreceiver/metrics_receiver.go	`0.00% <ø> (ø)`
receiver/zipkinreceiver/trace_receiver.go	`61.03% <ø> (ø)`
... and 55 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45c92ad...f77b7f1. Read the comment docs.

pjanotti

LGTM @tigrannajaryan

component/processor.go

component/extension.go

testbed/go.sum

bogdandrutu · 2020-03-23T20:58:50Z

I want to challenge this:

Rename old factories and components interfaces to use V1 suffix for clarity.

If we break now all the users, why keeping v1 and v2, I would suggest v2 to become the version without any suffix and the old interfaces maybe name the Old or just remove them completely.

tigrannajaryan · 2020-03-23T21:25:39Z

I want to challenge this:

Rename old factories and components interfaces to use V1 suffix for clarity.

If we break now all the users, why keeping v1 and v2, I would suggest v2 to become the version without any suffix and the old interfaces maybe name the Old or just remove them completely.

Yes, that's a possibility too. Let me play with the names a bit and see what it looks like.

tigrannajaryan · 2020-03-23T21:39:50Z

@bogdandrutu I added a new commit with the approach you suggested. PTAL.

tigrannajaryan · 2020-03-23T21:48:04Z

What everybody thinks about the general approach of putting all factories and interfaces in one package? Do you see any downsides? Like, e.g. the potential of creating too much coupling between normally unrelated the component factories?

tigrannajaryan · 2020-03-23T21:49:05Z

just remove them completely.

@bogdandrutu just to clarify, removing completely is not possible now. We have to first implemented the new interfaces in all components.

bogdandrutu · 2020-03-23T21:56:39Z

@tigrannajaryan the components package is very small and I would not worry that much of coupling as long as we don't put all the implementations there :)

Few questions/suggestions:

I am curious why consumer is outside, maybe that should be moved as well.
data is too generic for the new internal data package we should probably fix that before making it public.

tigrannajaryan · 2020-03-23T22:02:27Z

@tigrannajaryan the components package is very small and I would not worry that much of coupling as long as we don't put all the implementations there :)

Few questions/suggestions:
1. I am curious why consumer is outside, maybe that should be moved as well.

I'd keep this as a separate package. We will move new internal data representation implementation here once it is ready, so it will become a large package itself. I don't want to overload component package too much.

These are data models, they don't need any access to component factories / interfaces and I think it is better to keep them in a separate package. It is a typical approach to keep data model declarations as a separate package so that other packages can freely import them in any order without worrying about circular dependencies.

2. `data` is too generic for the new internal data package we should probably fix that before making it public.

Yes, these will need to be moved/renamed. But I want to wait until you finish your work and we have the implementation.

tigrannajaryan · 2020-03-24T15:09:59Z

@bogdandrutu any more thoughts on this?

owais

LGTM.

bogdandrutu · 2020-03-24T16:07:17Z

@tigrannajaryan for the moment I am done, will look again once it is merge (in some examples where we use this and see how it is).

jrcamp

lgtm

jrcamp · 2020-03-24T20:37:42Z

component/exporter.go

@@ -1,32 +1,60 @@
-// Copyright 2019, OpenTelemetry Authors
+// Copyright  OpenTelemetry Authors


nit: Is this supposed to have the year? Happened in a couple places.

As we are preparing for Beta release we want to cleanup publicly exported types and interfaces and do all necessary refactoring and breaking changes now, before the Beta release. We will have a lot less leeway for breaking changes after the Beta release. Component related type declarations are now all in the `component` package. This makes it possible for the interfaces to reference each other. This was were very restricted earlier because component interfaces were in 5 different packages and many proposals were impossible to implement because they would result in circular dependencies between packages. (An example upcoming new capability that is enabled by this refactoring is for components to query the Host for other components and for factories). List of changes in this commit: - Move all factory interfaces and component interfaces to component package. - Rename old factories and components interfaces to use "Old" suffix for clarity. - Eliminate forced checks that components implement factories. This is already enforced by the compiler when the factory is added to the Defaults() and was unnecessary code. - Eliminated some unnecessary codes (removed overall over 200 lines). - Run `go mod tidy` on testbed. Warning: this is a breaking change to publicly exported types and function signatures. We announced that a breaking change is comming. Once we agree to merge this commit we will need to announce the exact list of changes and guide component authors to modify their components accordingly. Future changes: - Once all components are migrated to the new internal representation, delete all "Old"-suffixed definitions. This will likely be done while we are still in Beta phase, before the Stable release.

fix interceptor regexp

tigrannajaryan requested review from bogdandrutu, flands, owais, pjanotti, rghetia and songy23 as code owners March 23, 2020 18:55

tigrannajaryan force-pushed the feature/tigran/refactor-components branch from 742508d to f77b7f1 Compare March 23, 2020 19:19

pjanotti approved these changes Mar 23, 2020

View reviewed changes

component/processor.go Outdated Show resolved Hide resolved

component/extension.go Outdated Show resolved Hide resolved

testbed/go.sum Outdated Show resolved Hide resolved

tigrannajaryan added the WIP label Mar 23, 2020

tigrannajaryan force-pushed the feature/tigran/refactor-components branch from f77b7f1 to b7a73e4 Compare March 23, 2020 21:29

tigrannajaryan force-pushed the feature/tigran/refactor-components branch from ee3cb7b to 4b0f347 Compare March 24, 2020 15:08

owais approved these changes Mar 24, 2020

View reviewed changes

tigrannajaryan force-pushed the feature/tigran/refactor-components branch 3 times, most recently from 2863d6e to 5d8d834 Compare March 24, 2020 17:55

tigrannajaryan removed the WIP label Mar 24, 2020

tigrannajaryan changed the title ~~[PROPOSAL] Refactor component and factory interface definitions~~ Refactor component and factory interface definitions Mar 24, 2020

jrcamp approved these changes Mar 24, 2020

View reviewed changes

tigrannajaryan force-pushed the feature/tigran/refactor-components branch from 5d8d834 to d792c20 Compare March 25, 2020 16:28

tigrannajaryan merged commit c931b98 into open-telemetry:master Mar 25, 2020

tigrannajaryan deleted the feature/tigran/refactor-components branch March 25, 2020 17:43

MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021

Merge pull request open-telemetry#683 from stefanprisca/master

8774926

fix interceptor regexp

hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023

Update Github actions rule for Ansible push (open-telemetry#683)

2be6932

swiatekm pushed a commit to swiatekm/opentelemetry-collector that referenced this pull request Oct 9, 2024

[demo] Remove arm warning from readme (open-telemetry#683)

1e47100

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor component and factory interface definitions #683

Refactor component and factory interface definitions #683

tigrannajaryan commented Mar 23, 2020 •

edited

Loading

codecov-io commented Mar 23, 2020

pjanotti left a comment

bogdandrutu commented Mar 23, 2020

tigrannajaryan commented Mar 23, 2020

tigrannajaryan commented Mar 23, 2020

tigrannajaryan commented Mar 23, 2020

tigrannajaryan commented Mar 23, 2020

bogdandrutu commented Mar 23, 2020

tigrannajaryan commented Mar 23, 2020

tigrannajaryan commented Mar 24, 2020

owais left a comment

bogdandrutu commented Mar 24, 2020

jrcamp left a comment

jrcamp Mar 24, 2020

		@@ -1,32 +1,60 @@
		// Copyright 2019, OpenTelemetry Authors
		// Copyright OpenTelemetry Authors

Refactor component and factory interface definitions #683

Refactor component and factory interface definitions #683

Conversation

tigrannajaryan commented Mar 23, 2020 • edited Loading

codecov-io commented Mar 23, 2020

Codecov Report

pjanotti left a comment

Choose a reason for hiding this comment

bogdandrutu commented Mar 23, 2020

tigrannajaryan commented Mar 23, 2020

tigrannajaryan commented Mar 23, 2020

tigrannajaryan commented Mar 23, 2020

tigrannajaryan commented Mar 23, 2020

bogdandrutu commented Mar 23, 2020

tigrannajaryan commented Mar 23, 2020

tigrannajaryan commented Mar 24, 2020

owais left a comment

Choose a reason for hiding this comment

bogdandrutu commented Mar 24, 2020

jrcamp left a comment

Choose a reason for hiding this comment

jrcamp Mar 24, 2020

Choose a reason for hiding this comment

tigrannajaryan commented Mar 23, 2020 •

edited

Loading