Skip to content

[common-go] Rate limiting on booleans and composite keys #17026

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

Merged
merged 2 commits into from
Mar 28, 2023

Conversation

WVerlaek
Copy link
Member

@WVerlaek WVerlaek commented Mar 24, 2023

Description

  • Support rate limiting keyed on boolean fields
  • Support composite rate limit keys
    • Comma separated, e.g. foo.bar,foo.baz
  • Improve performance of rate limiter by moving string splitting out of the hot path

Related Issue(s)

How to test

Run unit tests and benchmarks. Tested single and composite keys in a preview env

Release Notes

NONE

Documentation

Build Options:

  • /werft with-werft
    Run the build with werft instead of GHA
  • leeway-no-cache
  • /werft no-test
    Run Leeway with --dont-test
Publish Options
  • /werft publish-to-npm
  • /werft publish-to-jb-marketplace
Installer Options
  • with-ee-license
  • with-dedicated-emulation
  • with-ws-manager-mk2
  • workspace-feature-flags
    Add desired feature flags to the end of the line above, space separated

Preview Environment Options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

Comment on lines +89 to +93
fields := strings.Split(key, ",")
paths := make([][]string, len(fields))
for i, field := range fields {
paths[i] = strings.Split(field, ".")
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Extracting these string splits to outside the returned function improves performance by ~60%:

// OLD:
// BenchmarkFieldAccessKey_String-32                5800192               215.1 ns/op
// BenchmarkFieldAccessKey_Composite-32             3239696               371.9 ns/op
// NEW:
// BenchmarkFieldAccessKey_String-32                9338356               127.8 ns/op
// BenchmarkFieldAccessKey_Composite-32             5505349               217.8 ns/op

@WVerlaek WVerlaek force-pushed the wv/composite-rate-limit-key branch from 9ba5720 to 3502d81 Compare March 27, 2023 08:54
@WVerlaek WVerlaek force-pushed the wv/composite-rate-limit-key branch from 3502d81 to 9bd36a1 Compare March 27, 2023 15:47
@WVerlaek WVerlaek marked this pull request as ready for review March 27, 2023 15:48
@WVerlaek WVerlaek requested a review from a team as a code owner March 27, 2023 15:48
@WVerlaek
Copy link
Member Author

WVerlaek commented Mar 28, 2023

/hold blocking merge pool

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-wv-composite-rate-limit-key.5 because the annotations in the pull request description changed
(with .werft/ from main)

@WVerlaek
Copy link
Member Author

/unhold

@roboquat roboquat merged commit 4e3051d into main Mar 28, 2023
@roboquat roboquat deleted the wv/composite-rate-limit-key branch March 28, 2023 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Awaiting Deployment
Development

Successfully merging this pull request may close these issues.

4 participants