Release v1.4.0#86
Conversation
There was a problem hiding this comment.
Hey - 我在这里给了一些高层次的反馈:
- getCollector 每次被调用时都会启动两个长生命周期的 goroutine(包括一个 ticker);可以考虑只在初始化时启动一次,或者提供关闭机制,以避免在进程生命周期内多次调用 getCollector 时造成 goroutine 泄漏。
- doSyncDAU 会对每个 rid 顺序执行 PFCount 和 ZAdd,对于活跃集合很大的情况可能会比较慢;可以考虑使用 Redis pipeline 或 Lua 脚本将这些命令批量化,以减少往返次数。
- DAU 同步的 ticker 在每次 tick 时都会对当前日期调用 doSyncDAU,这可能会频繁地重新计算完整排名;请再次确认这种频率是否是有意为之,或者你是否只需要在日期切换时或更粗粒度的间隔进行同步。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- getCollector 每次被调用时都会启动两个长生命周期的 goroutine(包括一个 ticker);可以考虑只在初始化时启动一次,或者提供关闭机制,以避免在进程生命周期内多次调用 getCollector 时造成 goroutine 泄漏。
- doSyncDAU 会对每个 rid 顺序执行 PFCount 和 ZAdd,对于活跃集合很大的情况可能会比较慢;可以考虑使用 Redis pipeline 或 Lua 脚本将这些命令批量化,以减少往返次数。
- DAU 同步的 ticker 在每次 tick 时都会对当前日期调用 doSyncDAU,这可能会频繁地重新计算完整排名;请再次确认这种频率是否是有意为之,或者你是否只需要在日期切换时或更粗粒度的间隔进行同步。帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进评审质量。
Original comment in English
Hey - I've left some high level feedback:
- getCollector starts two long-lived goroutines (including a ticker) every time it is called; consider ensuring this is initialized only once or can be shut down to avoid leaking goroutines if getCollector is invoked multiple times over the process lifetime.
- doSyncDAU performs PFCount and ZAdd sequentially for every rid, which could be slow for large active sets; consider using Redis pipelining or a Lua script to batch these commands and reduce round-trips.
- The DAU sync ticker calls doSyncDAU for the current date on every tick, potentially recomputing the full ranking frequently; double-check whether this cadence is intentional or if you only need a sync on date rollover or at a coarser interval.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- getCollector starts two long-lived goroutines (including a ticker) every time it is called; consider ensuring this is initialized only once or can be shut down to avoid leaking goroutines if getCollector is invoked multiple times over the process lifetime.
- doSyncDAU performs PFCount and ZAdd sequentially for every rid, which could be slow for large active sets; consider using Redis pipelining or a Lua script to batch these commands and reduce round-trips.
- The DAU sync ticker calls doSyncDAU for the current date on every tick, potentially recomputing the full ranking frequently; double-check whether this cadence is intentional or if you only need a sync on date rollover or at a coarser interval.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR updates the request/Daily Active User (DAU) collection strategy for resources by shifting from per-request sorted-set increments to a batched background sync based on Redis HyperLogLogs and an “active resources” set, and bumps Fiber to the latest patch version.
Changes:
- Introduces Redis key prefixes/TTLs and a periodic sync loop to compute per-resource daily unique counts via
PFCOUNTand write them to a ZSET. - Reworks the collector to record activity in an “active resources” set per day and manage key expiration more efficiently.
- Updates
github.com/gofiber/fiber/v2fromv2.52.11tov2.52.12.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/interfaces/rest/handler/version.go | Reworks Redis-based DAU/request counting to use HLL + active set with a periodic sync to a sorted set. |
| go.mod | Bumps Fiber dependency version. |
| go.sum | Updates checksums for the Fiber version bump. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if _, ok := expiredHLL[val.rid]; !ok { | ||
| rdb.Expire(ctx, hllKey, keyTTL) | ||
| expiredHLL[val.rid] = struct{}{} | ||
| } |
There was a problem hiding this comment.
Expire results are ignored and the key is recorded as already-expired even if the Redis command fails. A transient Redis/network issue here can leave the HLL key without a TTL for the rest of the day (and potentially forever), since later events for the same rid won’t retry Expire. Consider checking the Expire error/return value and only caching the "expired" marker after success (or retrying periodically).
| if added > 0 { | ||
| if _, ok := expiredActive[date]; !ok { | ||
| rdb.Expire(ctx, activeKey, keyTTL) | ||
| expiredActive[date] = struct{}{} | ||
| } |
There was a problem hiding this comment.
Same issue as above for activeKey: Expire is not checked, but expiredActive is updated regardless. If Expire fails once, the daily active set may never get a TTL and can accumulate indefinitely. Handle the Expire result/error and only mark it as expired after a successful TTL set (or retry).
| rids, err := rdb.SMembers(ctx, activeKey).Result() | ||
| if err != nil { | ||
| h.logger.Warn("doSyncDAU SMembers error", zap.String("date", date), zap.Error(err)) | ||
| return | ||
| } | ||
| if len(rids) == 0 { | ||
| return | ||
| } | ||
|
|
||
| viewKey := viewKeyPrefix + date | ||
| for _, rid := range rids { | ||
| hllKey := hllKeyPrefix + rid + ":" + date | ||
| count, err := rdb.PFCount(ctx, hllKey).Result() | ||
| if err != nil { | ||
| h.logger.Warn("doSyncDAU PFCount error", zap.String("rid", rid), zap.Error(err)) | ||
| continue | ||
| } | ||
| if _, err := rdb.ZAdd(ctx, viewKey, redis.Z{ | ||
| Score: float64(count), | ||
| Member: rid, | ||
| }).Result(); err != nil { | ||
| h.logger.Warn("doSyncDAU ZAdd error", zap.String("rid", rid), zap.Error(err)) | ||
| } | ||
| } |
There was a problem hiding this comment.
SMembers reads the entire daily active set into memory and can be expensive/blocking on Redis if the set grows large. Consider iterating with SSCAN (or storing a bounded/partitioned structure) to avoid large allocations and long Redis response times.
| rids, err := rdb.SMembers(ctx, activeKey).Result() | |
| if err != nil { | |
| h.logger.Warn("doSyncDAU SMembers error", zap.String("date", date), zap.Error(err)) | |
| return | |
| } | |
| if len(rids) == 0 { | |
| return | |
| } | |
| viewKey := viewKeyPrefix + date | |
| for _, rid := range rids { | |
| hllKey := hllKeyPrefix + rid + ":" + date | |
| count, err := rdb.PFCount(ctx, hllKey).Result() | |
| if err != nil { | |
| h.logger.Warn("doSyncDAU PFCount error", zap.String("rid", rid), zap.Error(err)) | |
| continue | |
| } | |
| if _, err := rdb.ZAdd(ctx, viewKey, redis.Z{ | |
| Score: float64(count), | |
| Member: rid, | |
| }).Result(); err != nil { | |
| h.logger.Warn("doSyncDAU ZAdd error", zap.String("rid", rid), zap.Error(err)) | |
| } | |
| } | |
| viewKey := viewKeyPrefix + date | |
| var cursor uint64 | |
| for { | |
| rids, cur, err := rdb.SScan(ctx, activeKey, cursor, "*", 1000).Result() | |
| if err != nil { | |
| h.logger.Warn("doSyncDAU SScan error", zap.String("date", date), zap.Error(err)) | |
| break | |
| } | |
| for _, rid := range rids { | |
| hllKey := hllKeyPrefix + rid + ":" + date | |
| count, err := rdb.PFCount(ctx, hllKey).Result() | |
| if err != nil { | |
| h.logger.Warn("doSyncDAU PFCount error", zap.String("rid", rid), zap.Error(err)) | |
| continue | |
| } | |
| if _, err := rdb.ZAdd(ctx, viewKey, redis.Z{ | |
| Score: float64(count), | |
| Member: rid, | |
| }).Result(); err != nil { | |
| h.logger.Warn("doSyncDAU ZAdd error", zap.String("rid", rid), zap.Error(err)) | |
| } | |
| } | |
| if cur == 0 { | |
| break | |
| } | |
| cursor = cur | |
| } |
| for _, rid := range rids { | ||
| hllKey := hllKeyPrefix + rid + ":" + date | ||
| count, err := rdb.PFCount(ctx, hllKey).Result() | ||
| if err != nil { | ||
| h.logger.Warn("doSyncDAU PFCount error", zap.String("rid", rid), zap.Error(err)) | ||
| continue | ||
| } | ||
| if _, err := rdb.ZAdd(ctx, viewKey, redis.Z{ | ||
| Score: float64(count), | ||
| Member: rid, | ||
| }).Result(); err != nil { | ||
| h.logger.Warn("doSyncDAU ZAdd error", zap.String("rid", rid), zap.Error(err)) | ||
| } | ||
| } |
There was a problem hiding this comment.
The sync loop performs one PFCount + one ZAdd round-trip per rid, every 15 minutes. This can become a significant Redis/network load as the active set grows. Consider batching with a pipeline (or Lua) and/or syncing only newly-active rids since the last run (e.g., a "dirty" set that gets cleared after processing).
| for _, rid := range rids { | |
| hllKey := hllKeyPrefix + rid + ":" + date | |
| count, err := rdb.PFCount(ctx, hllKey).Result() | |
| if err != nil { | |
| h.logger.Warn("doSyncDAU PFCount error", zap.String("rid", rid), zap.Error(err)) | |
| continue | |
| } | |
| if _, err := rdb.ZAdd(ctx, viewKey, redis.Z{ | |
| Score: float64(count), | |
| Member: rid, | |
| }).Result(); err != nil { | |
| h.logger.Warn("doSyncDAU ZAdd error", zap.String("rid", rid), zap.Error(err)) | |
| } | |
| } | |
| // First pipeline: batch PFCount for all rids | |
| pfPipe := rdb.Pipeline() | |
| pfCountCmds := make([]*redis.IntCmd, len(rids)) | |
| for i, rid := range rids { | |
| hllKey := hllKeyPrefix + rid + ":" + date | |
| pfCountCmds[i] = pfPipe.PFCount(ctx, hllKey) | |
| } | |
| if _, err := pfPipe.Exec(ctx); err != nil { | |
| h.logger.Warn("doSyncDAU PFCount pipeline exec error", zap.String("date", date), zap.Error(err)) | |
| return | |
| } | |
| // Second pipeline: batch ZAdd using PFCount results | |
| zaddPipe := rdb.Pipeline() | |
| for i, rid := range rids { | |
| count, err := pfCountCmds[i].Result() | |
| if err != nil { | |
| h.logger.Warn("doSyncDAU PFCount error", zap.String("rid", rid), zap.Error(err)) | |
| continue | |
| } | |
| zaddPipe.ZAdd(ctx, viewKey, redis.Z{ | |
| Score: float64(count), | |
| Member: rid, | |
| }) | |
| } | |
| if _, err := zaddPipe.Exec(ctx); err != nil { | |
| h.logger.Warn("doSyncDAU ZAdd pipeline exec error", zap.String("date", date), zap.Error(err)) | |
| } |
Summary by Sourcery
使用 Redis 优化每日活跃使用量收集机制,并定期同步聚合计数,同时更新一个框架依赖。
新特性:
增强:
构建:
Original summary in English
Summary by Sourcery
Refine daily active usage collection using Redis and periodically synchronize aggregated counts while updating a framework dependency.
New Features:
Enhancements:
Build: