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

fix: Throughput processor should be singleton #1842

Merged
merged 10 commits into from
Sep 6, 2024

Conversation

BinaryFissionGames
Copy link
Member

@BinaryFissionGames BinaryFissionGames commented Sep 5, 2024

Proposed Change

  • Make individual throughput processors a singleton across multiple pipelines.
  • If a throughput measurement processor is registered twice, throw an error.
  • Fix issue where empty metrics were emitted for every 0 valued metric.
  • Don't log every time we send metrics.

We were seeing some issues with measurements in BPOP. This was because when the same processor was included in multiple pipelines, it would create multiple instances. In that case, one processor would "win" the registration, and only measurements for that particular pipeline would be emitted.

This change makes it so that there is only one instance of the processor (this pattern is copied from the snapshot processor), so all the pipelines are accounted for. You can see that here in BPOP, where we have multiple sources (which end up being multiple pipelines) adding up correctly.

Screenshot 2024-09-05 at 11 21 34 AM
Checklist
  • Changes are tested
  • CI has passed

@BinaryFissionGames BinaryFissionGames force-pushed the fix/custom-message-throughput-incorrect branch from 097cc93 to f3c77ba Compare September 5, 2024 17:36
@BinaryFissionGames BinaryFissionGames marked this pull request as ready for review September 5, 2024 17:38
@BinaryFissionGames BinaryFissionGames requested review from jsirianni and antonblock and removed request for jsirianni September 5, 2024 17:38
Copy link
Contributor

@antonblock antonblock left a comment

Choose a reason for hiding this comment

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

Working well with the custom message change enabled in BP, just had a couple small comments

internal/measurements/throughput.go Show resolved Hide resolved
internal/measurements/throughput_test.go Outdated Show resolved Hide resolved
internal/measurements/throughput_test.go Outdated Show resolved Hide resolved
internal/measurements/throughput_test.go Outdated Show resolved Hide resolved
@BinaryFissionGames BinaryFissionGames changed the base branch from release/v1.60.0 to main September 6, 2024 18:20
@BinaryFissionGames BinaryFissionGames force-pushed the fix/custom-message-throughput-incorrect branch from bb25f58 to dd8595b Compare September 6, 2024 18:21
@BinaryFissionGames BinaryFissionGames merged commit 5f2bb9c into main Sep 6, 2024
15 checks passed
@BinaryFissionGames BinaryFissionGames deleted the fix/custom-message-throughput-incorrect branch September 6, 2024 18:56
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.

2 participants