fix(static/metrics/instance): fix duplicate metrics registration panic when recreating the instance #6608
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…c when recreating the instance Signed-off-by: hainenber <dotronghai96@gmail.com>
hainenber
commented
Mar 5, 2024
| @@ -278,7 +278,6 @@ func TestInstance_Recreate(t *testing.T) { | |||
|
|
|||
| // Recreate the instance, no panic should happen. | |||
| require.NotPanics(t, func() { | |||
| inst, err := New(prometheus.NewRegistry(), cfg, walDir, logger) | |||
There was a problem hiding this comment.
This actually recreates a blank new Prom registry and the children write handler won't have any duplicate registration, thus no panic.
Removing it allows for reproduction of the panic
There was a problem hiding this comment.
I don't think we should remove this line (the comment doesn't recreate the instance anymore), but rather pass in the same registry on both line 261 and here. Can we add it back and make that change?
There was a problem hiding this comment.
And I can confirm that without the change in instance.go, that also reproduces the panic.
…r-registration-panic
rfratto
reviewed
Mar 5, 2024
| @@ -278,7 +278,6 @@ func TestInstance_Recreate(t *testing.T) { | |||
|
|
|||
| // Recreate the instance, no panic should happen. | |||
| require.NotPanics(t, func() { | |||
| inst, err := New(prometheus.NewRegistry(), cfg, walDir, logger) | |||
There was a problem hiding this comment.
I don't think we should remove this line (the comment doesn't recreate the instance anymore), but rather pass in the same registry on both line 261 and here. Can we add it back and make that change?
hainenber
changed the title
…with past Prom registry Signed-off-by: hainenber <dotronghai96@gmail.com>
hainenber
deleted the
fix-write-handler-duplicate-metrics-collector-registration-panic
branch
rfratto
pushed a commit
to rfratto/agent
that referenced
this pull request
Mar 5, 2024
…c when recreating the instance (grafana#6608) Signed-off-by: hainenber <dotronghai96@gmail.com> (cherry picked from commit 7a61067)
rfratto
added a commit
that referenced
this pull request
Mar 5, 2024
* Set permissions on the Grafana Agent [Flow] folder... (#6540) * Set permissions on the folder when installing via the windows installer rather than relying on the parent folder permissions. Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> --------- Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> (cherry picked from commit 9e4d3b5) * Loader reuse existing nodes on reload (#6288) * Move UpdateBlock from componentNode interface to blockNode interface. This change means that all blockNodes have now the possibility to update their managed block with the update River block content. * Update the loader to update the managed block of a config node on reload if it already existed in the graph. With this optimization, we re-use existing nodes and update them instead of creating a new node. This is especially useful for modules. * add two new tests to check that on reload the config nodes behave as expected * add updateblock to declare node * update loader logic to detect duplicated blocks and reuse already defined blocks * add updateblock to import config node * update changelog * move function and remove unnecessary check (cherry picked from commit 2e9d5a2) * fix(static/metrics/instance): fix duplicate metrics registration panic when recreating the instance (#6608) Signed-off-by: hainenber <dotronghai96@gmail.com> (cherry picked from commit 7a61067) * chore(build): upgrade base image to frequently updated ECR-hosted Ubuntu (#6612) Signed-off-by: hainenber <dotronghai96@gmail.com> (cherry picked from commit fe513a4) * prepare for 0.40.2 release (#6619) (cherry picked from commit ed54148) * Port promtail changes part 1 (#6559) * Port promtail changes part 1 * changelog (cherry picked from commit 1a642cf) * remove accidentally committed web/ui/build folder * fix issue with cherry-picking CHANGELOG.md * fix bad conflict resolution * fix test which failed due to bad merge conflict resolution * On new windows installs, remove default read permissions from agent c… (#6622) * On new windows installs, remove default read permissions from agent config Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * only apply permissions for a new install Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * Update CHANGELOG.md Co-authored-by: Robert Fratto <robertfratto@gmail.com> --------- Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> Co-authored-by: Robert Fratto <robertfratto@gmail.com> (cherry picked from commit e8a3d29) --------- Co-authored-by: Erik Baranowski <39704712+erikbaranowski@users.noreply.github.com> Co-authored-by: William Dumont <william.dumont@grafana.com> Co-authored-by: Đỗ Trọng Hải <41283691+hainenber@users.noreply.github.com> Co-authored-by: Piotr <17101802+thampiotr@users.noreply.github.com>
github-actions
bot
added
the
frozen-due-to-age
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description
Props to @rfratto for troubleshooting and guidance, I'm just implementing the idea here :D
Which issue(s) this PR fixes
Fixes #6548
Notes to the Reviewer
PR Checklist