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

chore: Migrate redigo to go-redis, upgrade throttled #2737

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mschfh
Copy link
Collaborator

@mschfh mschfh commented Jan 21, 2025

Summary

This PR replaces our outdated redigo dependency with go-redis.

Type of Change

  • Product feature
  • Bug fix
  • Performance improvement
  • Refactor
  • Other

Tested Environments

  • Development
  • Staging
  • Production

Before Requesting Review

  • Does your code build cleanly without any errors or warnings?
  • Have you used auto closing keywords?
  • Have you added tests for new functionality?
  • Have validated query efficiency for new database queries?
  • Have documented new functionality in README or in comments?
  • Have you squashed all intermediate commits?
  • Is there a clear title that explains what the PR does?
  • Have you used intuitive function, variable and other naming?
  • Have you requested security and/or privacy review if needed
  • Have you performed a self review of this PR?

@mschfh mschfh self-assigned this Jan 21, 2025
@mschfh mschfh force-pushed the mschfh-goredis branch 2 times, most recently from b8273b3 to 9db4ff3 Compare January 22, 2025 10:05
@mschfh mschfh marked this pull request as ready for review January 22, 2025 10:07
@mschfh mschfh requested a review from clD11 January 22, 2025 10:08
Copy link
Contributor

@clD11 clD11 left a comment

Choose a reason for hiding this comment

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

LGTM approved with comments.

libs/clients/coingecko/client.go Outdated Show resolved Hide resolved
libs/clients/coingecko/client.go Show resolved Hide resolved
libs/clients/coingecko/client.go Outdated Show resolved Hide resolved
libs/clients/coingecko/client_test.go Outdated Show resolved Hide resolved
libs/clients/coingecko/client_test.go Outdated Show resolved Hide resolved
libs/middleware/rate_limiter.go Outdated Show resolved Hide resolved
libs/middleware/rate_limiter.go Outdated Show resolved Hide resolved
services/ratios/cache.go Show resolved Hide resolved
services/ratios/cache.go Outdated Show resolved Hide resolved
services/ratios/cache.go Outdated Show resolved Hide resolved
Copy link

[puLL-Merge] - brave-intl/bat-go@2737

Description

This PR updates Redis and rate limiter dependencies to newer versions and improves Docker build caching. The main changes involve:

  1. Migrating from the legacy gomodule/redigo Redis client to the modern redis/go-redis/v9
  2. Upgrading the rate limiter from throttled to throttled/v2
  3. Adding Docker build cache for go mod downloads
Changes

Changes

Dockerfile

  • Added build cache for go mod downloads using --mount=type=cache
  • Standardized stage naming convention to uppercase AS

Redis Client Changes

  • Changed Redis client from gomodule/redigo to redis/go-redis/v9 across multiple packages
  • Updated all Redis operations to use new client APIs and context support
  • Removed manual connection management code as go-redis handles pooling internally

Rate Limiter Changes

  • Updated from throttled to throttled/v2 package
  • Migrated to newer context-aware APIs (GCRAStoreCtx, HTTPRateLimiterCtx, etc.)
  • Updated rate limiter middleware to use new APIs

Dependency Updates

  • Added various new Redis deps like dgryski/go-rendezvous
  • Updated go.mod and go.sum files across multiple packages
  • Removed old Redis and rate limiter dependencies
sequenceDiagram
    participant App
    participant RateLimiter
    participant Redis
    
    App->>RateLimiter: HTTP Request
    RateLimiter->>Redis: Check Rate Limit (with context)
    Redis-->>RateLimiter: Return Current Count
    alt Under Limit
        RateLimiter->>App: Allow Request
    else Over Limit
        RateLimiter->>App: Return 429 Too Many Requests
    end
Loading

Security Hotspots

  • Redis security considerations have not materially changed as connection security is handled through connection strings/URLs
  • Rate limiting security remains consistent but now has proper context support for cancellation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants