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

feat(experiment): make drop decision queue size configurable #1472

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

VinozzZ
Copy link
Contributor

@VinozzZ VinozzZ commented Jan 21, 2025

Which problem is this PR solving?

The cuckoo_addqueue_full shows full when experimenting HPA. I would like to increase the queue size since now each refinery will hold all trace decisions in the cluster instead of just 1/Nth

benchmark with the queue size increased from 1000 to 10000

goos: darwin
goarch: arm64
pkg: github.com/honeycombio/refinery/collect/cache
cpu: Apple M2 Max
BenchmarkCuckooTraceChecker_Add-12                 	268866240	        8.203 ns/op	      0 B/op	      0 allocs/op
BenchmarkCuckooTraceChecker_AddParallel-12         	1957152	      603.1 ns/op	      2 B/op	      0 allocs/op
BenchmarkCuckooTraceChecker_Check-12               	47585214	       25.77 ns/op	      0 B/op	      0 allocs/op
BenchmarkCuckooTraceChecker_CheckParallel-12       	1930326	      615.8 ns/op	      0 B/op	      0 allocs/op
BenchmarkCuckooTraceChecker_CheckAddParallel-12    	 967039	     1296 ns/op	     11 B/op	      0 allocs/op

Short description of the changes

  • add DroppedQueueSize config option
  • add incoming/peer_router_otlp metric to track otlp traffic

@VinozzZ VinozzZ self-assigned this Jan 21, 2025
@VinozzZ VinozzZ requested a review from a team as a code owner January 21, 2025 17:47
@VinozzZ VinozzZ changed the title feat: make drop decision queue size configurable feat(experiment): make drop decision queue size configurable Jan 21, 2025
@VinozzZ VinozzZ force-pushed the yingrong/make_drop_decision_queue_configurable branch from f08fdf0 to 7bd123b Compare January 21, 2025 22:14
@@ -50,13 +50,16 @@ var cuckooTraceCheckerMetrics = []metrics.Metadata{
{Name: CurrentLoadFactor, Type: metrics.Gauge, Unit: metrics.Percent, Description: "the fraction of slots occupied in the current cuckoo filter"},
}

func NewCuckooTraceChecker(capacity uint, m metrics.Metrics) *CuckooTraceChecker {
func NewCuckooTraceChecker(capacity uint, addQueueDepth uint, m metrics.Metrics) *CuckooTraceChecker {
Copy link
Contributor

Choose a reason for hiding this comment

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

addQueueDepth param isn't used, should it be used on line 62?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch 😮‍💨

route/route.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these changes related to drop queue size or fixing that bug where we don't increment the right metrics for OTLP ingest during stress relief?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, this PR is my experimental PR. I'm not planning to merge it. I shouldn't have marked it for review

@VinozzZ VinozzZ marked this pull request as draft January 22, 2025 20:43
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