Skip to content

Use MustRegisterOrGet to avoid possible panics in remotecfg#4101

Open
dehaansa wants to merge 5 commits intomainfrom
mustregisterorget
Open

Use MustRegisterOrGet to avoid possible panics in remotecfg#4101
dehaansa wants to merge 5 commits intomainfrom
mustregisterorget

Conversation

@dehaansa
Copy link
Contributor

@dehaansa dehaansa commented Aug 1, 2025

PR Description

Use MustRegisterOrGet to avoid unnecessary panics.

This won't resolve issues for components like pyroscope where the registration code is in a different go module. For those, we would have to consider wrapping the registry we pass to the component in the getManagedOptions/getImportManagedOptions functions in the controller modules with the same code from util, which is probably worth considering as another safety check.

Which issue(s) this PR fixes

Fixes #2801

Copy link
Contributor

@matthewnolf matthewnolf left a comment

Choose a reason for hiding this comment

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

LGTM for database_observability component

@korniltsev
Copy link
Contributor

Can you somehow enforce MustRegisterOrGet and forbid args.Registry.MustRegister? Or at least a comment next to each MustRegisterOrGet describing why it is needed with a link to an issue or PR. Maybe a lint or some other check. I would never write myself a util.MustRegisterOrGet and would very much likely break this again every time I'd had a new metric.

Can you explain why do panics occur in the description of the PR?

@dehaansa
Copy link
Contributor Author

dehaansa commented Aug 4, 2025

Can you somehow enforce MustRegisterOrGet and forbid args.Registry.MustRegister? Or at least a comment next to each MustRegisterOrGet describing why it is needed with a link to an issue or PR. Maybe a lint or some other check. I would never write myself a util.MustRegisterOrGet and would very much likely break this again every time I'd had a new metric.

Can you explain why do panics occur in the description of the PR?

I don't know if it can be enforced via a linter rule or similar, will investigate. The majority of the issues related come down to validation of components after failure to launch and remotecfg issues. It may be worth a deeper investigation instead of this knee-jerk reaction, I think the majority of it comes down to getting the timing right - only registering metrics after we're sure a component or module is good, versus registering when we initialize a component but we're not sure it's going to run.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2025

This PR has not had any activity in the past 90 days, so the needs-attention label has been added to it.
If you do not have enough time to follow up on this PR or you think it's no longer relevant, consider closing it.
The needs-attention label signals to maintainers that something has fallen through the cracks. No action is needed by you; your PR will be kept open and you do not have to respond to this comment. The label will be removed the next time this job runs if there is new activity.
Thank you for your contributions!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

misconfigured component is created twice duplicate metrics collector registration panic when using remotecfg

3 participants