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

Should we move component.TelemetrySettings to not be embedded? #11045

Closed
Tracked by #11005
mx-psi opened this issue Sep 4, 2024 · 12 comments
Closed
Tracked by #11005

Should we move component.TelemetrySettings to not be embedded? #11045

mx-psi opened this issue Sep 4, 2024 · 12 comments

Comments

@mx-psi
Copy link
Member

mx-psi commented Sep 4, 2024

component.TelemetrySettings is currently an embedded field in all Settings structs for receivers, processors, exporters, connectors and extensions, while all other fields in those structs are not. I believe this is just for historical reasons: we started with just the logger and to ease the transition for components we embedded the field instead of adding a top-level field.

Should we un-embed it? The main advantage is namespacing: we would avoid clashes if we want to add new fields. I feel like we shouldn't do this because:

  1. It's very unlikely we would run into clashes with other fields
  2. It's a bit of an annoying breaking change since it's hard to search and replace for it cleanly
  3. We can't really make this change in two steps
@atoulme
Copy link
Contributor

atoulme commented Sep 4, 2024

I vote to leave as is.

@TylerHelmuth
Copy link
Member

I dont see enough end-user or component developers value in changing it now. I vote we leave it as is.

@codeboten
Copy link
Contributor

I agree that we should leave this as is, I don't see the benefits in changing it.

@codeboten codeboten reopened this Sep 5, 2024
@codeboten
Copy link
Contributor

Accidentally closed 🤦🏻, reopened.

@bogdandrutu
Copy link
Member

I would remove if not a huge deal. To do it in 2 steps I would just duplicate the data in the components Settings

Settings {
  component.TelemetrySettings
  Telemetry component.TelemetrySettings
}

And when initialize in the Service we initialize both.

@mx-psi
Copy link
Member Author

mx-psi commented Sep 6, 2024

Would we then do a third step for renaming Telemetry to TelemetrySettings?

@mx-psi
Copy link
Member Author

mx-psi commented Sep 11, 2024

I tested on #11113 this change to see the breakage. It broke 5/10 components (~50%) in core and 129/196 components (~66%) I tested in contrib (after fixing mdatagen code). Given that this cannot be fixed with a simple search and replace but rather needs going through issues one by one, I think it would be quite annoying for component developers.

I suggest we close this as wontfix.

@mx-psi
Copy link
Member Author

mx-psi commented Sep 12, 2024

I will close this by end of next week unless somebody has a way to make the migration easier for users.

@jmacd
Copy link
Contributor

jmacd commented Sep 16, 2024

Please do not.

@TylerHelmuth
Copy link
Member

@jmacd please do not close or please do not change?

@mx-psi
Copy link
Member Author

mx-psi commented Sep 19, 2024

I interpreted @jmacd as "please do not change" (as a reply to the issue title).

I am going to close this as wontfix, but it would be good to get confirmation from them (will reach out in Slack)

@mx-psi mx-psi closed this as not planned Won't fix, can't repro, duplicate, stale Sep 19, 2024
@jmacd
Copy link
Contributor

jmacd commented Sep 19, 2024

Sorry I meant please don't change the embedding of telemetry settings-- it causes a release incompatibility for little benefit IMO. All open PRs have to rerun all the CI workflows after fixing in flight work, and I'm already spending a lot of time waiting on PRs that could merge if it were not for frequent updates and broken tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

6 participants