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

pkg/profiler: hack to fix double-registration panic #253

Merged
merged 1 commit into from Feb 24, 2022

Conversation

Sylfrena
Copy link
Contributor

@Sylfrena Sylfrena commented Feb 23, 2022

Signed-off-by: Sumera Priyadarsini sylphrenadin@gmail.com

Fix #255

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Good catch 👍 It's going in the right direction.

cmd/parca-agent/main.go Outdated Show resolved Hide resolved
cmd/parca-agent/main.go Outdated Show resolved Hide resolved
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

When that re-review this, after @metalmatze and I talked, I think we shouldn't pass the counter while we already passing the reg. We just need to add a label.

While this does fix the double registration panic issue,
further work needs to be done to expose relevant labels.

Signed-off-by: Sumera Priyadarsini <sylphrenadin@gmail.com>
@Sylfrena Sylfrena force-pushed the fix-double-registration-panic branch from 6f19aed to 1b854c2 Compare February 24, 2022 14:55
@Sylfrena Sylfrena marked this pull request as ready for review February 24, 2022 15:10
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM

@kakkoyun kakkoyun changed the title [WIP]pkg/profiler: hack to fix double-registration panic pkg/profiler: hack to fix double-registration panic Feb 24, 2022
@kakkoyun kakkoyun merged commit 9d37ad7 into main Feb 24, 2022
@kakkoyun kakkoyun deleted the fix-double-registration-panic branch February 24, 2022 15:13
@brancz
Copy link
Member

brancz commented Feb 24, 2022

I apologize for only getting to this now, but I don't think this is 100% correct. The problem is that this metric never gets unregistered when the profiler is stopped. I believe the only way to fix this correctly is to pass the CounterVec from the thing that creates the profiler, call WithLabelValues in the NewProfiler function and then remove the counter with the DeleteLabelValues when stopping the profiler.

@Sylfrena Sylfrena restored the fix-double-registration-panic branch February 28, 2022 07:53
@kakkoyun kakkoyun deleted the fix-double-registration-panic branch February 28, 2022 15:12
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.

Duplicate metrics collector registration attempted
3 participants