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.

Sign up for GitHub

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

Jump to bottom

[extension/receiver/exporter/processor/connector] Should we remove component.Type from Settings.ID? #12221

Open
mx-psi opened this issue Jan 31, 2025 · 8 comments · May be fixed by #12381
Open

[extension/receiver/exporter/processor/connector] Should we remove component.Type from Settings.ID? #12221

mx-psi opened this issue Jan 31, 2025 · 8 comments · May be fixed by #12381
Assignees
@mx-psi
Labels
area:connector area:exporter area:extension area:processor area:receiver
Milestone
go.opentelemetry....

Comments

@mx-psi
Copy link
Member

mx-psi commented Jan 31, 2025

The component.Type bit from Settings.ID is redundant for components: a component knows its own component.Type.

Should we remove component.Type from Settings.ID?

I think not, but I want to do a prototype, and see how it looks.

From the 2025-01-27 stability working group meeting.
@mx-psi
Copy link
Member Author

mx-psi commented Feb 5, 2025

I took a look at this. I think one important datapoint here is that there are:

The searches are approximations (it's very hard to get an exact match) but I think it clearly shows that there are a lot more usages of the ID directly rather than just its name.

In summary: I think we should not do this because:

  1. While there is repetition, it seems justified based on the fact that the component.ID is used as-is much more frequently than the component name, which is used very rarely.
  2. This would be a major breaking change, we use set.ID a lot in contrib, and therefore I would expect other users outside of contrib also use it a lot.

@mx-psi
Copy link
Member Author

mx-psi commented Feb 5, 2025

@open-telemetry/collector-approvers I intend to close this as wontfix on Friday unless somebody objects.

@bogdandrutu
Copy link
Member

bogdandrutu commented Feb 5, 2025 •
edited
Loading

While there is repetition, it seems justified based on the fact that the component.ID is used as-is much more frequently than the component name, which is used very rarely.

There is a simple script to replace that: component.NewID(metadata.Type, set.Name) will be exactly that. Or we can even create a function ID(component.Type) and will use set.ID(metadata.Type)

@mx-psi
Copy link
Member Author

mx-psi commented Feb 5, 2025

While there is repetition, it seems justified based on the fact that the component.ID is used as-is much more frequently than the component name, which is used very rarely.

There is a simple script to replace that: component.NewID(metadata.Type, set.Name) will be exactly that. Or we can even create a function ID(component.Type) and will use set.ID(metadata.Type)

I am not saying it can't be done, I am just saying that that would mean users having to figure out how to build an ID. If 90% of the time users want to just use the ID, why not just give that to them?

@bogdandrutu
Copy link
Member

If 90% of the time users want to just use the ID, why not just give that to them?

Not saying to not give the ID, or a function like ID() in setting to the user. The fundamental question I have is which component sets the Type part.

@mx-psi mx-psi mentioned this issue Feb 6, 2025
Merged
@mx-psi
Copy link
Member Author

mx-psi commented Feb 6, 2025 •
edited
Loading

@bogdandrutu My point is that it's okay if we have this duplication since it leads to a better user experience.

I understand the concern of mismatch because of multiple packages setting the type, one possible solution is to explicitly check for it. I filed #12305 to do that for extension; if that makes sense we can extend it to other component types and close this issue.

github-merge-queue bot pushed a commit that referenced this issue Feb 7, 2025
@mx-psi
[extension] Error out if passed extension.Settings has incorrect type (…
183d7ed 
…#12305)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Explicitly error out if the passed `component.ID` does not have a
matching `component.Type`

<!-- Issue number if applicable -->
#### Link to tracking issue
Updates #12221
@mx-psi
Copy link
Member Author

mx-psi commented Feb 10, 2025

Moved this to the receiver milestone since this is done for extension now

@mx-psi mx-psi self-assigned this Feb 10, 2025
@mx-psi mx-psi moved this from Todo to In Progress in Collector: v1 Feb 10, 2025
@mx-psi mx-psi moved this from In Progress to Waiting for reviews in Collector: v1 Feb 10, 2025
@mx-psi mx-psi moved this from Waiting for reviews to In Progress in Collector: v1 Feb 11, 2025
@mx-psi mx-psi mentioned this issue Feb 11, 2025
Merged
github-merge-queue bot pushed a commit that referenced this issue Feb 11, 2025
@mx-psi
Deprecate 'Create*' methods (#12356)
fc644ed 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

<!-- Issue number if applicable -->

Deprecate `Create*` methods. This is a small part of #12305, but applied
to all other component kinds

#### Link to tracking issue
Updates #12221
@mx-psi mx-psi mentioned this issue Feb 13, 2025
Merged
@mx-psi
Copy link
Member Author

mx-psi commented Feb 13, 2025

#12357 is the next step to do this on all component kinds

@mx-psi mx-psi linked a pull request Feb 13, 2025 that will close this issue
Open
github-merge-queue bot pushed a commit that referenced this issue Feb 13, 2025
@mx-psi
[*test] Add NewNopSettingsWithType everywhere (#12357)
a4ae175 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

<!-- Issue number if applicable -->

Creates `NewNopSettingsWithType` function on test modules and deprecates
the `NewNopSettings` functions on those modules. Replace all usages of
`NewNopSettings` with `NewNopSettingsWithType`.

Part of #12305 but applied to all component kinds.

#### Link to tracking issue
Updates #12221
mx-psi added a commit to mx-psi/opentelemetry-collector that referenced this issue Feb 14, 2025
@mx-psi
[*test] Add NewNopSettingsWithType everywhere (open-telemetry#12357)
bceb46a 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

<!-- Issue number if applicable -->

Creates `NewNopSettingsWithType` function on test modules and deprecates
the `NewNopSettings` functions on those modules. Replace all usages of
`NewNopSettings` with `NewNopSettingsWithType`.

Part of open-telemetry#12305 but applied to all component kinds.

#### Link to tracking issue
Updates open-telemetry#12221
@mx-psi mx-psi mentioned this issue Feb 18, 2025
Merged
mx-psi added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this issue Feb 19, 2025
@mx-psi
[chore] Use NewNopSettingsWithType(metadata.Type) on tests (#38030)
de6f834 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Tries to fix some of the contrib tests for
open-telemetry/opentelemetry-collector/pull/12381

<!-- Issue number (e.g. #1234) or full URL to issue, if applicable. -->
#### Link to tracking issue

Updates open-telemetry/opentelemetry-collector/issues/12221
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mx-psi mx-psi

Labels
area:connector area:exporter area:extension area:processor area:receiver
Projects
Collector: v1
Status: In Progress
Milestone
go.opentelemetry.io/receiver 1.0
Development

Successfully merging a pull request may close this issue.

[connector,exporter,processor,receiver] Error out on mismatched type mx-psi/opentelemetry-collector
2 participants
@bogdandrutu @mx-psi