Skip to content

fix(metrics): update FSC to adopt provider-side collector caching#1907

Merged
adecaro merged 1 commit into
LFDT-Panurus:mainfrom
Built-by-Sign:perf/ttx-finality-listener-metrics-once
Jul 17, 2026
Merged

fix(metrics): update FSC to adopt provider-side collector caching#1907
adecaro merged 1 commit into
LFDT-Panurus:mainfrom
Built-by-Sign:perf/ttx-finality-listener-metrics-once

Conversation

@EvanYan1024

@EvanYan1024 EvanYan1024 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Motivation

Metric collectors are requested per finality listener and per TMS, and the FSC prometheus provider used to swallow duplicate registrations (SkipRegisterErr: true): later requesters silently received an unregistered collector, so their observations never showed up on /metrics, and every request paid a registry round-trip under the registry lock. We hit both in our test environments under sustained load.

Per the review discussion here, the deduplication was moved down into the smart client: hyperledger-labs/fabric-smart-client#1582 (merged) caches collectors inside the prometheus provider, so every consumer is protected. This PR adopts it and removes the token-sdk-side workaround.

Changes

Tests

Fixes #1906

@adecaro
adecaro force-pushed the perf/ttx-finality-listener-metrics-once branch from 1e5db57 to 2b66f80 Compare July 14, 2026 03:55
@adecaro
adecaro self-requested a review July 14, 2026 03:55
@adecaro adecaro self-assigned this Jul 14, 2026
@adecaro adecaro added this to the Q3/26 milestone Jul 14, 2026
@adecaro

adecaro commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Hi @EvanYan1024 , thanks for submitting this.

What about a more radical solution that modifies the metrics.Provider implementation to cache the metrics and return the ones that exist already? I fear that we might have similar issues elsewhere.

What do you think?

@EvanYan1024

Copy link
Copy Markdown
Contributor Author

Agreed, this direction is better — pushing the deduplication down into the provider as a single choke point fixes the whole class of issues in one go, not just these finality collectors.

@EvanYan1024
EvanYan1024 force-pushed the perf/ttx-finality-listener-metrics-once branch 2 times, most recently from a244c3e to 2608bce Compare July 14, 2026 08:51
@EvanYan1024 EvanYan1024 changed the title perf(ttx): register finality listener metrics once and share the instance fix(metrics): cache collectors in the provider to deduplicate registrations Jul 14, 2026
@EvanYan1024
EvanYan1024 force-pushed the perf/ttx-finality-listener-metrics-once branch from 2608bce to e27bc75 Compare July 14, 2026 09:01
@adecaro

adecaro commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

I think @mbrandenburger can be interested in this one. If we inject the caching mechanism directly in the smart client, we are protected also there.

@EvanYan1024

Copy link
Copy Markdown
Contributor Author

@adecaro Moved the fix into the smart client's provider as suggested: hyperledger-labs/fabric-smart-client#1582 (issue hyperledger-labs/fabric-smart-client#1581). The provider now caches collectors by fully-qualified name, so repeated requests share the collector registered first and skip the registry entirely. Once that lands, this PR can be closed or reduced to a dependency bump.

@adecaro

adecaro commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Hi @EvanYan1024 , so hyperledger-labs/fabric-smart-client#1582 has been merged. As soon as FSC cuts a new tag, we can update also panurus and we should be good. What do you think?

@EvanYan1024

Copy link
Copy Markdown
Contributor Author

Sounds good @adecaro! Once FSC cuts the tag I'll update this PR: drop the decorator caching, bump FSC, and adapt the operations.NewMetricsProvider call in integration/nwo/runner/rpc/executor.go (the skipRegisterErr parameter was removed in FSC #1582). While at it, recoverFromDuplicate in the TMS provider becomes dead code and can be removed too.

@EvanYan1024
EvanYan1024 force-pushed the perf/ttx-finality-listener-metrics-once branch from 83d7151 to 4daa56c Compare July 17, 2026 03:05
@EvanYan1024 EvanYan1024 changed the title fix(metrics): cache collectors in the provider to deduplicate registrations fix(metrics): update FSC to adopt provider-side collector caching Jul 17, 2026
@EvanYan1024

EvanYan1024 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

@adecaro I've gone ahead and reworked this PR: it now bumps FSC to the hyperledger-labs/fabric-smart-client#1582 merge commit (v0.14.3-0.20260716084917-c10decb043ec), drops the decorator and the recoverFromDuplicate workaround, and adapts to the NewMetricsProvider signature change. Once the new FSC tag is cut I'll swap the pseudo-version to it — or feel free to merge as-is if the pseudo-version works for you.

hyperledger-labs/fabric-smart-client#1582 caches collectors inside the
prometheus provider, fixing duplicate registration at the source. Bump
FSC across modules, drop the tmsProvider recover workaround, and adapt
to the NewMetricsProvider signature change.

Signed-off-by: Evan <[email protected]>
@adecaro
adecaro force-pushed the perf/ttx-finality-listener-metrics-once branch from 4daa56c to 8cdfa21 Compare July 17, 2026 05:28
@adecaro

adecaro commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Thanks @EvanYan1024. We have a much robust solution now. Great effort 🙏

@adecaro adecaro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@adecaro
adecaro merged commit 4aac80b into LFDT-Panurus:main Jul 17, 2026
91 checks passed
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.

Finality listener metrics are re-registered per listener and per TMS; later registrations become unscraped collectors

2 participants