Checklist for new components

[jpkrohling]

I have authored a few components already and reviewed a few more, but it's not always clear to me what should be included and where. I can imagine that others have the same problem. The contributing guidelines help in this direction, but it's still a bit limited. This issue aims to capture a checklist for new components, with the goal of updating the contributing guidelines with this information.

Before any code is written:

• Get a sponsor for your component. A sponsor is an approver who will be in charge of being the official reviewer of the code. For vendor-specific components, it's good to have a volunteer sponsor. If you can't find one, we'll assign one in a round-robin fashion. For non-vendor components, having a sponsor means that your use case has been validated.

First PR:

• Mention the sponsor in the PR description
• A complete config.go, which will show how your component will look like to users
• README.md with some basic information about your component. It doesn't have to be complete, but should contain at least:
  • Purpose and use-cases
  • Telemetry data types supported
• versions.yaml
• CODEOWNERS with at least the sponsor and the component's author
• internal/components tests
• go.mod at the root of the repository and cmd/configschema
• dependabot
• The skeleton of the component, potentially using the helpers
• Makefile

Subsequent PRs, except the last one:

• Implement the logic. There is no need to implement it all at once, do it in small chunks to make it easier to review.

Last PR:

• Component observability. We are still blocked on this, and it is being waived, see Document how component developers can add metrics to their code opentelemetry-collector#4198.
• Wire up the component in the components.go. This will go away in the future, probably in favor of a manifest in the same repo, similar to what we have for the core distribution (the one that is built upon make run)

On the -releases repo:

• Add the component to the manifest: https://github.com/open-telemetry/opentelemetry-collector-releases/blob/main/distributions/otelcol-contrib/manifest.yaml

[jpkrohling]

cc @MovieStoreGuy, @open-telemetry/collector-contrib-approvers, I'm sure I'm missing something here. I would appreciate your input.

[mx-psi]

It could be a good idea to turn this checklist into an issue template, and require a tracking issue for every new component

[MovieStoreGuy]

+1 on having this as an issue template for adding a new component, I think that would really help with contribution experience and what is expected of the authors.

I do have some questions on what is the limitations of components to be added:

• Does the component's associated service need some level of critical mass before it can be considered to be part of the contrib repo?
• Can a component be deprecated due to lack of support / maintenance ?
• Should we consider adopters voting for new components to be added?

I do worry how adhoc some components are added to the collector that it is hard to set a clear roadmap, and I fully understand that this is still a new space, and vendors / tools are coming up to help but it does muddy what potential roadmaps can be set out.

I do not want to limit contributors but at some point it is going to be hard to scale development without impacting devx.

[jpkrohling]

There are vendor-specific components, and components not tied to specific vendors. The vendor-specific ones should always be allowed: either we have all vendors or no vendors.

Components can indeed be removed from the contrib distribution: if a component gets orphaned and bugs are not getting fixed, it should be removed.

Should we consider adopters voting for new components to be added?

That's a good idea.

[anuraaga]

Personal opinion, I've never really loved our current guidelines of splitting PRs for components which seems similar to what's proposed here because

• First PR ends up basically being a rubber stamp PR. The second PR tends to be massive anyways, and splitting out rubber-stamp type content doesn't help me that much, I can skim them and focus more on the code even within a single PR without the second PR feeling any more overwhelming
• A PR with only boilerplate often stubs out empty methods for interface implementations and it can be hard to follow what is meant to be reviewed as final code and what is in-progress waiting for the second PR
• GitHub will often assign a different reviewer for first and second PRs, making any context lost

Could separate commits based on this well-defined scheme within one PR work similarly without some of those drawbacks?

[jpkrohling]

I also don't like having multiple PRs for pretty much the same reasons you listed (see open-telemetry/opentelemetry-collector#1952), but given we do have this as part of the guidelines and is enforced for most PRs, it is included as part of this checklist.

That said, I would support a change in the policy if you'd open an issue with the proposal.

[djaglowski]

Does the component's associated service need some level of critical mass before it can be considered to be part of the contrib repo?
...
Should we consider adopters voting for new components to be added?

I agree with the notion that we need to protect against ad hoc components but I think the sponsorship process already does much to ensure this. We could slightly enhance the process by clarifying that another approver must "second" the addition of the component. Often this can be a maintainer who is not the sponsor, so this keeps unnecessary friction to a minimum.

I'd prefer not to require a voting step, since this will add significant friction to an already cumbersome contribution process. Ultimately, if a component is accepted when it possibly should not have been, one can always open an issue to propose removing it.

[djaglowski]

I would support a change in the policy if you'd open an issue with the proposal.

+1

I understand the motivation to make PRs more reviewable, but given the investment we expect of sponsors it seems reasonable that we should be willing to immerse ourselves in larger PRs. The current process is very non-trivial to contributors who have already drafted a complete component.

[Aneurysm9]

I also strongly dislike having new components split into multiple PRs. It makes them harder to review in my opinion because we are forced to accept the existence of a new component before we know what its implementation will look like. Suggesting that commit history be used to partition the implementation for reviewers who prefer incremental reviews might work, and it is probably how most people are attempting to satisfy the current process anyways.

[Aneurysm9]

That said, I would support a change in the policy if you'd open an issue with the proposal.

open-telemetry/opentelemetry-collector#4630

[leonsp-ai]

First PR: go.mod at the root of the repository and cmd/configschema

I think that would include the stub of the new component in the build. Should this go in the last PR instead, assuming there are multiple PRs?

[bogdandrutu]

@jpkrohling there is also some current documentation that may not be up-to-date https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#adding-new-components

[jpkrohling]

Based on this issue's description, here's a new version, based on what we discussed during the latest SIG meeting:

Before any code is written, open an issue providing the following information:

• Who's the sponsor for your component. A sponsor is an approver who will be in charge of being the official reviewer of the code. For vendor-specific components, it's good to have a volunteer sponsor. If you can't find one, we'll assign one in a round-robin fashion. For non-vendor specific components, having a sponsor means that your use case has been validated.
• Some basic information about your component, such as the reasoning behind it, use-cases, telemetry data types supported, and anything else you think it's relevant for us to make a decision about accepting the component
• The configuration options your component will accept. This will help us understand what it does and have an idea of how the implementation might look like

Once we agree that your component should be included, send in a PR including:

• README.md, potentially with the same or similar information from the issue mentioned above
• versions.yaml
• CODEOWNERS with at least the sponsor and the component's author
• Tests in internal/components
• go.mod at the root of the repository and cmd/configschema
• dependabot
• Makefile
• Component's implementation
• Component observability. We are still blocked on this, and it is being waived, see Document how component developers can add metrics to their code opentelemetry-collector#4198.

After the initial implementation is merged:

• Add the component to the manifest: https://github.com/open-telemetry/opentelemetry-collector-releases/blob/main/distributions/otelcol-contrib/manifest.yaml

In order to make it easier to review this PR, try to get individual commits in logical steps:

• skeleton/scaffolding in the first commit
• implementation in the subsequent commits, each in a reasonable chunk

We prefer to have a single PR with an initial version of your component in a functioning state. But it doesn't mean that you should include all the possible options from day 1: if you intend on providing an interface with multiple implementations, consider providing only one implementation at first.

[bogdandrutu]

I think the issue cannot have the sponsor initially, I see the flow as open the issue with details and then identify a sponsor (approvers can nominate themselves or round robin per your comments). If a sponsor is identified then we can proceed to PRs, otherwise no PRs

[Aneurysm9]

I think the issue cannot have the sponsor initially

"cannot", or "can not"? It seems that it should be fine to open an issue with a sponsor already identified, but that it isn't required to have a sponsor before opening an issue.

[jpkrohling]

Should this be closed? I think we have the guidelines already in place:
https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#adding-new-components