Skip to content

Add positional aggregator interface - #423

Open
pedro-stanaka wants to merge 7 commits into
mainfrom
pedro-stanaka/positional-aggregator-api
Open

Add positional aggregator interface#423
pedro-stanaka wants to merge 7 commits into
mainfrom
pedro-stanaka/positional-aggregator-api

Conversation

@pedro-stanaka

@pedro-stanaka pedro-stanaka commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Why

Aggregation is currently hard-coded to the built-in implementation and receives metric options as keyword arguments. The aggregator is an internal hot-path interface, so a single fixed-arity positional contract is simpler, cheaper to dispatch, and easier to implement with alternative aggregation strategies.

What changed

  • Make the internal aggregator interface positional-only:

    increment(name, value, tags, no_prefix, sample_rate)
    gauge(name, value, tags, no_prefix)
    aggregate_timing(name, value, tags, no_prefix, type, sample_rate)
  • Keep the public Client metric API unchanged, including its keyword options.

  • Add optional aggregator: injection to Client.new and .from_env.

  • Preserve an injected aggregator through with_options / clone_with_options unless explicitly overridden.

  • Providing aggregator: implicitly enables aggregation.

  • Convert untagged aggregation calls to one shared frozen empty array; other tag values are forwarded as-is, so custom implementations may require canonical arrays.

  • Keep sampling in Client before aggregation dispatch.

  • Keep the existing precompiled aggregation methods and flush as part of the aggregator contract.

  • Document the interface and add a changelog entry.

Compatibility

Public StatsD and Client metric methods keep their existing signatures and behavior. Code that directly calls or prepends the internal Aggregator methods must update to the positional signatures.

Injected aggregators own prefix/default-tag policy and lifecycle behavior. Cloned clients reuse the injected instance unless aggregator: is overridden.

Performance

Linux arm64, Ruby 3.4.10 + YJIT, 1M calls:

Path Median Ruby allocations/call
Direct positional aggregator method 24.6 ns/call 0
Client → aggregator 32.5 ns/call 0

A five-process before/after run found no regression in existing direct or CompiledMetric paths. Representative medians:

Path main this branch
Client distribution 1,351.9 ns 1,321.8 ns
Dynamic compiled distribution 184.5 ns 178.7 ns
Static compiled distribution 133.7 ns 134.5 ns
Dynamic compiled counter 216.6 ns 214.2 ns

Validation

  • bundle exec rake test — 397 runs, 1,111 assertions, 0 failures/errors
  • bundle exec rubocop — 79 files, no offenses
  • Ruby 2.7–3.3, ruby-head, JRuby, and TruffleRuby CI coverage
  • UDP integration test

#gsd:52109

@pedro-stanaka
pedro-stanaka force-pushed the pedro-stanaka/positional-aggregator-api branch from 9e8a5f0 to 8c5f144 Compare July 21, 2026 20:47
@pedro-stanaka
pedro-stanaka marked this pull request as ready for review July 21, 2026 21:06
@pedro-stanaka
pedro-stanaka force-pushed the pedro-stanaka/positional-aggregator-api branch from 6467d3c to c67ae86 Compare August 17, 2026 18:07
@pedro-stanaka
pedro-stanaka marked this pull request as draft August 17, 2026 18:07
@pedro-stanaka
pedro-stanaka force-pushed the pedro-stanaka/positional-aggregator-api branch from 96473eb to c67ae86 Compare August 18, 2026 09:00
pedro-stanaka and others added 7 commits August 18, 2026 11:11
Assisted-By: devx/2d33ac09-1dc1-4608-a505-946e99eed108
Assisted-By: devx/2d33ac09-1dc1-4608-a505-946e99eed108
Co-authored-by: AI (Pi/GLM 5.2 Fast (Fireworks) [300k]) <noreply@pi.dev>
Assisted-By: devx/2d33ac09-1dc1-4608-a505-946e99eed108
- Remove KeywordTestAggregator: call Aggregator positionally in tests
- Remove PositionalAggregator mock: use real Aggregator with CaptureSink
- Delete test_default_aggregator_uses_positional_dispatch (implementation detail)
- Delete test_aggregator_entrypoints_use_fixed_positional_arguments (already removed)

Co-authored-by: AI (Pi/GLM 5.2 Fast (Fireworks) [300k]) <noreply@pi.dev>
Assisted-By: devx/2d33ac09-1dc1-4608-a505-946e99eed108
Co-authored-by: AI (Pi/GLM 5.2 Fast (Fireworks) [300k]) <noreply@pi.dev>
Assisted-By: devx/2d33ac09-1dc1-4608-a505-946e99eed108
Co-authored-by: AI (Pi/GLM 5.2 Fast (Fireworks) [300k]) <noreply@pi.dev>
Assisted-By: devx/2d33ac09-1dc1-4608-a505-946e99eed108
Co-authored-by: AI (Pi/GLM 5.2 Fast (Fireworks) [300k]) <noreply@pi.dev>
Assisted-By: devx/2d33ac09-1dc1-4608-a505-946e99eed108
@pedro-stanaka
pedro-stanaka force-pushed the pedro-stanaka/positional-aggregator-api branch from c67ae86 to eec17f5 Compare August 18, 2026 09:12
@pedro-stanaka
pedro-stanaka marked this pull request as ready for review August 18, 2026 10:00
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.

1 participant