Skip to content

Fixing Duplicate Metrics Registration when Unsetting preInit #389

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
Merged
merged 2 commits into from
Jan 7, 2022
Merged

Fixing Duplicate Metrics Registration when Unsetting preInit #389

merged 2 commits into from
Jan 7, 2022

Conversation

@onelapahead
Copy link
Contributor

@onelapahead onelapahead commented Jan 6, 2022 •
edited
Loading

The Prometheus registry panics when metrics are duplicated twice. With an ff bug fix in v0.0.41, @nguyer noticed firefly starts running with just the admin server, and then when it gets set to run both the admin server and the REST server without restarting the process, the admin middleware metrics would get re-registered causing a panic:

panic: duplicate metrics collector registration attempted

goroutine 242 [running]:
github.com/prometheus/client_golang/prometheus.(*wrappingRegisterer).MustRegister(0xc000453ad8, 0xc000453b00, 0x4, 0x4)
/go/pkg/mod/github.com/prometheus/[email protected]/prometheus/wrap.go:104 +0x185
gitlab.com/hfuss/mux-prometheus/pkg/middleware.(*Instrumentation).initMetrics(0xc000012a00)
    /go/pkg/mod/gitlab.com/hfuss/[email protected]/pkg/middleware/middleware.go:112 +0x66a
gitlab.com/hfuss/mux-prometheus/pkg/middleware.NewCustomInstrumentation(0xc0005f6801, 0xf51b74, 0xc, 0xf48b3d, 0x5, 0x157cce0, 0xb, 0xb, 0xc0005f68a0, 0x1086cf8, ...)
    /go/pkg/mod/gitlab.com/hfuss/[email protected]/pkg/middleware/middleware.go:58 +0x13c
github.com/hyperledger/firefly/internal/apiserver.(*apiServer).configurePrometheusInstrumentation(0xc0001adf80, 0xf51b74, 0xc, 0xf48b3d, 0x5, 0xc0002cc600)

this refactors the instrumentation creation and middleware registration in the routers so that the original instrumentations are always re-used whenever router are recreated in-process to prevent the duplicate registration.

@onelapahead
less duplication
95fca22 
Signed-off-by: hfuss <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2022 •
edited
Loading

Codecov Report

Merging #389 (95fca22) into main (73b1a2b) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #389   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          239       239           
  Lines        12929     12920    -9     
=========================================
- Hits         12929     12920    -9
Impacted Files Coverage Δ
internal/apiserver/server.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73b1a2b...95fca22. Read the comment docs.

nguyer
nguyer approved these changes Jan 7, 2022
View reviewed changes
Copy link
Contributor

@nguyer nguyer left a comment

Choose a reason for hiding this comment

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

Thanks @hfuss! Also worth noting that this PR updates the version of the CLI used in E2E which validates the fix itself. Thanks for doing that.

@nguyer nguyer merged commit 4a09178 into hyperledger:main Jan 7, 2022
@nguyer nguyer deleted the fixing-prom-middleware-admin-api branch January 7, 2022 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@awrichar awrichar Awaiting requested review from awrichar

@peterbroadhurst peterbroadhurst Awaiting requested review from peterbroadhurst

1 more reviewer

@nguyer nguyer nguyer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@onelapahead @codecov-commenter @nguyer