augur icon indicating copy to clipboard operation
augur copied to clipboard

keyman performance improvements

Open shlokgilda opened this issue 1 month ago • 2 comments

Background

Working with the keyman orchestration system and found a few issues that I think should be addressed.

Issues

Key distribution isn't fair When you publish the same key to the same platform twice, it gets added to the pool multiple times and receives extra traffic. Not common but should be prevented.

# Without duplicate check, key appears twice in pool
keypub.publish("ghp_abc123", "github_rest")
keypub.publish("ghp_abc123", "github_rest")  # duplicate
# Output: fresh_keys["github_rest"] = ["ghp_abc123", "ghp_abc123"]

Typo in exception class WaitKeyTimeout has tiemout_seconds instead of timeout_seconds. Found this while debugging.

Missing documentation

  • No clear docs on how many Redis connections keyman creates
  • The directory could benefit from a directory-specific README.

What I'm planning to fix

  1. Add a duplicate check in publish_key() - simple if key not in list before appending
  2. Fix the typo
  3. Document the connection pooling situation
  4. Improve docstrings
  5. Write a proper README for the keyman directory

Testing

Should be straightforward. Publish the same key twice and make sure it only appears once in the pool

shlokgilda avatar Nov 17 '25 04:11 shlokgilda

as noted in #3414, i think the duplicate publishing was intentionally a feature because probabiltically weihted key use was something that was used previously but isnt anymore. if we want to remove this, it may be good to store keys as a set, ehich gaurantees they are unique

MoralCode avatar Dec 03 '25 20:12 MoralCode

In that case, I can open up a new PR that stores the keys in a set. Or even better, should I open a new issue that we can mark as "good first issue" for others to pick up?

shlokgilda avatar Dec 06 '25 00:12 shlokgilda

Based on the title, your existing, closed PR for this may update docs and a typo, so let's maybe merge those two pieces since they exist already (i assume). Then we can just use this issue as the "duplicate key prevention" issue I guess

In general combining multiple problems into one issue isn't ideal, but i'm also guilty of doing that myself when the additional fixes are small (like the docs and typos)

MoralCode avatar Dec 15 '25 20:12 MoralCode

Re-opened PR #3414 . The new PR now only deals with typo fix and doc changes. Removed the duplicate key addition check code from that PR @MoralCode

shlokgilda avatar Dec 18 '25 17:12 shlokgilda