Skip to content

Conversation

@georgi
Copy link
Collaborator

@georgi georgi commented Dec 17, 2025

Convert ResultsFulfillerMetrics from an object to a class that accepts a Meter parameter. This allows tests to inject their own meter instance for better test isolation and prevents test pollution from shared global state.

  • Convert ResultsFulfillerMetrics from object to class
  • Add create() factory method for default Instrumentation meter
  • Add metrics parameter to ResultsFulfiller constructor
  • Update ResultsFulfillerAppRunner to create metrics instance

@georgi georgi requested a review from Marco-Premier December 17, 2025 15:14
@wfa-reviewable
Copy link

This change is Reviewable

@georgi georgi requested a review from kungfucraig December 17, 2025 15:14
@georgi georgi marked this pull request as ready for review December 17, 2025 15:15
Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

@kungfucraig reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @georgi and @Marco-Premier)


src/main/kotlin/org/wfanet/measurement/edpaggregator/resultsfulfiller/ResultsFulfiller.kt line 106 at r1 (raw file):

  private val fulfillerSelector: FulfillerSelector,
  private val responsePageSize: Int? = null,
  private val metrics: ResultsFulfillerMetrics = ResultsFulfillerMetrics.create(),

I'd prefer if we didn't default this. It causes long-term maintenance headaches.

Copy link
Collaborator Author

@georgi georgi left a comment

Choose a reason for hiding this comment

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

@georgi reviewed 1 of 3 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kungfucraig and @Marco-Premier)


src/main/kotlin/org/wfanet/measurement/edpaggregator/resultsfulfiller/ResultsFulfiller.kt line 106 at r1 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

I'd prefer if we didn't default this. It causes long-term maintenance headaches.

Done.

Convert ResultsFulfillerMetrics from an object to a class that accepts
a Meter parameter. This allows tests to inject their own meter instance
for better test isolation and prevents test pollution from shared global state.

- Convert ResultsFulfillerMetrics from object to class
- Add create() factory method for default Instrumentation meter
- Add metrics parameter to ResultsFulfiller constructor
- Update ResultsFulfillerAppRunner to create metrics instance
@georgi georgi force-pushed the georgi/metrics-test-isolation branch 2 times, most recently from 920f2c7 to 32ef289 Compare December 17, 2025 21:29
@georgi georgi force-pushed the georgi/metrics-test-isolation branch from 32ef289 to a155516 Compare December 18, 2025 09:49
Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

@kungfucraig reviewed 6 files and all commit messages, and resolved 1 discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Marco-Premier).

@georgi georgi merged commit d5135c2 into main Dec 18, 2025
5 checks passed
@georgi georgi deleted the georgi/metrics-test-isolation branch December 18, 2025 19:13
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.

4 participants