Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move cmd/otel/components to internal/components #4604

Merged
merged 4 commits into from
Aug 19, 2021

Conversation

jjackson-s
Copy link
Contributor

Description:
Moving functionality - There is currently functionality to get all of the components in both core and contrib inside of cmd/otel/components, and it is also inaccessible because it is saved under a main package.

  • Thought it would be useful to move it to internal/components to make it accessible and easier to find.

Link to tracking Issue:

  • No tracking issue, but the file is inaccessible outside of cmd/otel, and is something that can be useful to the entire repo.

Testing:

  • No new testing, but there is currently a linker error being thrown due to using both defaultcomponents and the components that we are local to contrib. I am currently working on resolving that issue, but am not sure exactly from where it is coming from. Also present when directory lives inside of cmd/otel/components.

Documentation:
n/a

@jjackson-s jjackson-s requested review from a team and tigrannajaryan August 11, 2021 23:53
@tigrannajaryan
Copy link
Member

  • Thought it would be useful to move it to internal/components to make it accessible and easier to find.

Why does it need to be accessible? What's the intended use case?

@jjackson-s
Copy link
Contributor Author

  • Thought it would be useful to move it to internal/components to make it accessible and easier to find.

Why does it need to be accessible? What's the intended use case?

  • First, I am working on getting a project merged into contrib that relies on this change add Configwiz #4592
  • But other use cases would be if there is anybody in the future that wants access to both the components inside of contrib and core for any future works. Very similar to default components in core repo ./service/defaultcomponents.

@tigrannajaryan
Copy link
Member

Build failed:

2021-08-13T13:54:55.2084916Z make[2]: Entering directory '/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib'
2021-08-13T13:55:03.4447757Z make[2]: Leaving directory '/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/testbed'
2021-08-13T13:55:04.0255796Z golangci-lint run --allow-parallel-runners
2021-08-13T13:55:09.1733581Z make[2]: Leaving directory '/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/tracegen'
2021-08-13T13:55:17.3647703Z make[2]: Leaving directory '/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/receiver/simpleprometheusreceiver'
2021-08-13T13:56:39.2522456Z cmd/otelcontribcol/unstable_components_disabled.go:23:6: `extraReceivers` is unused (deadcode)
2021-08-13T13:56:39.2523482Z func extraReceivers() []component.ReceiverFactory {
2021-08-13T13:56:39.2524162Z      ^
2021-08-13T13:56:39.2869715Z make[2]: *** [Makefile.Common:93: lint] Error 1
2021-08-13T13:56:39.2872264Z make[1]: *** [Makefile:153: for-all-target-/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib] Error 2
2021-08-13T13:56:39.2874243Z make[2]: Leaving directory '/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib'
2021-08-13T13:56:39.2876098Z make[1]: Leaving directory '/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib'
2021-08-13T13:56:39.2877485Z make: *** [Makefile:81: golint] Error 2
2021-08-13T13:56:39.2887705Z ##[error]Process completed with exit code 2.

@@ -143,7 +143,7 @@ func components() (component.Factories, error) {
udplogreceiver.NewFactory(),
}

receivers = append(receivers, extraReceivers()...)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where extraReceivers() was once called

Copy link
Contributor

@rmfitzpatrick rmfitzpatrick Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the same, and refers to the enabled unstable components if the build tag is set: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/ac315e8e921efeba1565a6dddec1f7c09fa00667/cmd/otelcontribcol/unstable_components_enabled.go

edit: It's not being used now, but as new unstable components are added there's the ability to enable them conditionally without being locked into this file's contents.

second edit: missed your related comment. Is there a reason moving them to this new internal package is problematic?

Copy link
Contributor Author

@jjackson-s jjackson-s Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, its made this way for components that have yet to merged into contrib correct?

But because the unstable components files are apart of the main package, I won't have access to them when moving components.go to internal.

If I am understanding correctly though, you are saying to move over the unstable_components files over to internal as well correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're for components that have been added to the contrib project (and are in the standard receiver, exporter, etc. directories) but haven't been added to the (previously) main component factories for vetting and reliability reasons. By building a collector distribution with the unstable tag are they available. Once hardened to a reasonable extent they get moved from the unstable_enabled location to the applicable slice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow ok that makes sense I was wondering about that unstable tag. I was thinking that makefiles were only used for testing purposes but it seems that it can also be used to help compilation maybe? Definitely going to look deeper into it but thanks for that explanation!

@jjackson-s
Copy link
Contributor Author

jjackson-s commented Aug 16, 2021

cmd/otelcontribcol/unstable_components_disabled.go:23:6: extraReceivers is unused (deadcode)
2021-08-13T13:56:39.2523482Z func extraReceivers() []component.ReceiverFactory {

Ok so I looked into the failing test, and as stated in the error we are not using this extraReceivers function, but I am not sure what we should do with it.

  • extraReceivers() was called before to add extra receivers, and there are 2 versions of its function (one returns nil and the other returns an empty component.ReceiverFactory{})
  • both untouched in the entire repo, living in cmd/otelcontribcol/unstable_components_disabled.go and cmd/otelcontribcol/unstable_components_enabled.go.
  • Due to the extraReceiver() functions residing in cmd/otelcontribcol, we do not have access to these functions any longer because they live in a main package instead of a named, exportable one.
  • The two ideas of what we can do with it would that I can think of would either be to remove it altogether or to export the function, but let me know what you think.

@jjackson-s jjackson-s changed the title moved cmd/otel/components -> internal/components move cmd/otel/components to internal/components Aug 17, 2021
@bogdandrutu
Copy link
Member

Please fix the lint errors

@bogdandrutu bogdandrutu merged commit bff12af into open-telemetry:main Aug 19, 2021
Aneurysm9 pushed a commit that referenced this pull request Aug 19, 2021
* moved cmd/otel/components -> internal/components

* move unstable_components to internal

* update Makefile
hex1848 pushed a commit to hex1848/opentelemetry-collector-contrib that referenced this pull request Jun 2, 2022
…emetry#4604)

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.42.0 to 1.43.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.42.0...v1.43.0)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants