Store reported piped stats to redis #2190
Conversation
|
Code coverage for golang is
|
pipecd-bot
left a comment
pipecd-bot
left a comment
There was a problem hiding this comment.
Some issues were detected while linting go source files in your changes.
|
Code coverage for golang is
|
pipecd-bot
left a comment
pipecd-bot
left a comment
There was a problem hiding this comment.
Some issues were detected while linting go source files in your changes.
pipecd-bot
left a comment
pipecd-bot
left a comment
There was a problem hiding this comment.
Some issues were detected while linting go source files in your changes.
|
Code coverage for golang is
|
khanhtc1202
changed the title
pipecd-bot
left a comment
pipecd-bot
left a comment
There was a problem hiding this comment.
Some issues were detected while linting go source files in your changes.
pkg/app/api/pipedstatstore/store.go
Outdated
| } | ||
|
|
||
| // TODO: Implement GetCurrentStats so that ops can use it to expose pipeds' stat to prometheus. | ||
| func (c *store) GetCurrentStats() ([]byte, error) { |
There was a problem hiding this comment.
receiver name c should be consistent with previous receiver name s for store
|
Code coverage for golang is
|
|
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use Details1. Implement GetCurrentStats so that ops can use it to expose pipeds' stat to prometheus.This was created by todo plugin since "TODO:" was found in badf024 when #2190 was merged. cc: @khanhtc1202. |
|
Code coverage for golang is
|
khanhtc1202
changed the title
pkg/cache/cache.go
Outdated
| // Putter wraps a method to write to cache. | ||
| type Putter interface { | ||
| Put(key interface{}, value interface{}) error | ||
| PutHash(key interface{}, value interface{}) error |
There was a problem hiding this comment.
Not sure why do we need this? I think we can reuse Put function.
There was a problem hiding this comment.
Just in case we want to use RedisHashCache.Put(...) interface, I think we should keep the existing Put implementation for RedisHashCache. 🤔
There was a problem hiding this comment.
Just in case we want to use RedisHashCache.Put(...) interface
Can you explain more about this? That is for putting a set of (field, value) for a key?
There was a problem hiding this comment.
That is for putting a set of (field, value) for a key?
Since the current RedisHashCache has embedded RedisCache instance, in the caller side, we can use RedisHashCache.Get(key) which call to RedisHashCache.RedisCache.Get(key) with the current implementation. In that case, It will return an error not found (even if we Put the (k,v) pair before) if we override the Put interface with our PutHash implementation. That why I think we should keep the existing Put interface and add PutHash which be used for the new purpose of storing (k,v) as values of the defined hashkey of RedisHashCache.
There was a problem hiding this comment.
I think we should not embed RedisCache. We can either add a new cache: RedisCache field or use redis.Redis directly.
I think we have no reason to put PutHash function to all cache interfaces.
There was a problem hiding this comment.
Well, if I understand correctly, I feel like the current RedisCache is enough. We can just use piped-key as a key and use a stats byte sequence as a value. Tell me why using HSET if any.
There was a problem hiding this comment.
I think that is why GetAll and RedisHashCache were introduced. We can have all stats of all pipeds by a single command in a fast way to expose for prometheus.
There was a problem hiding this comment.
🙆♀️ Get you guys' point 🙏
Addressed on 478ee0d 👀
At this implementation, I change all implementation for Cache interface of RedisHashCache as it will treat the Hash object stored in redis as its backend and replace all XXX commands with HXXX commands.
There was a problem hiding this comment.
alright, makes sense. it's for retrieving all stats in bulk.
There was a problem hiding this comment.
Tell me why using HSET if any.
With only the single SET interface, we need a way to know all possible piped (ID) currently registered to the controlplane and query for the data one by one which much slower, I guess 👀
pkg/app/api/pipedstatstore/store.go
Outdated
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package pipedstatstore |
There was a problem hiding this comment.
How about passing the cache directly to the api instance instead of creating this store package?
Here are some reasons:
- This store is containing only cache
- Ops also needs this, but this is putting inside
apipackage
There was a problem hiding this comment.
You're right 👍 lets me address this 🙏
cmd/pipecd/server.go
Outdated
| cmds := commandstore.NewStore(ds, cache, t.Logger) | ||
| is := insightstore.NewStore(fs) | ||
| cmdOutputStore := commandoutputstore.NewStore(fs, t.Logger) | ||
| hcache := rediscache.NewTTLHashCache(rd, cfg.Cache.TTLDuration(), defaultPipedStatHashKey) |
|
Code coverage for golang is
|
pkg/cache/rediscache/hashcache.go
Outdated
| return err | ||
| } | ||
|
|
||
| func (r *RedisHashCache) GetAll() ([]interface{}, error) { |
There was a problem hiding this comment.
Who uses GetAll() currently? Any plan to use it later?
There was a problem hiding this comment.
Okay, this PR is focusing on storeing
|
Code coverage for golang is
|
| _, pipedID, _, err := rpcauth.ExtractPipedToken(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
How about adding a simple guard here to ensure that the sent data is not exceeding the maximum size we can accept?
Just to protect the stat cache.
There was a problem hiding this comment.
Sure 👍 I wonder which limit we should set it to 🤔
Suppose we have 100 pipeds commit its stats, then it will be 51.2MB for each? 🤔
A String value can be at max 512 Megabytes in length.
There was a problem hiding this comment.
I think 1MB is enough.
But I just remembered that gRPC is also having its validation on the message size.
Its default value is 4MB. So it is good as well.
So I think we don't have to add any additional code, just leave to use the default value.
There was a problem hiding this comment.
Gotcha 👌
Another thing I have concerned about is the number of "field" in this hash. As mentioned in the docs
A hash with a few fields (where few means up to one hundred or so) is stored in a way that takes very little space, so you can store millions of objects in a small Redis instance.
We may have a limit of up to hundreds of piped which could be registered to controlplane 👀 Will open an issue for that later since up to this time, we don't have any limitation around how may piped can be controlled by controlplane.
There was a problem hiding this comment.
You are right. We may split the Piped list into smaller groups like grouping by projectID.
Nice to have an issue about the field number.
pkg/cache/rediscache/hashcache.go
Outdated
| func (r *RedisHashCache) GetAll() ([]interface{}, error) { | ||
| conn := r.redis.Get() | ||
| defer conn.Close() | ||
| reply, err := redigo.StringMap(conn.Do("HGETALL", r.key)) |
There was a problem hiding this comment.
I think we can not use StringMap in this case.
Because HGETALL is returning an array that containing both field and value.
So we have to use Array function to parse the reply. https://pkg.go.dev/github.com/gomodule/redigo/redis#Values
One more point is that instead of returning []interface{} maybe we should return map[interface{}]interface{}.
There was a problem hiding this comment.
If we adopt map[interface{}]interface{} as return value, then the current redigo.StringMap is good enough since its convert reply to map[string]string, how do you think about that 🤔
There was a problem hiding this comment.
Adopted returning map[interface{}]interface{} idea on d7e89e0 🙆♀️
It looks much better now, thx 👍
|
Code coverage for golang is
|
|
Code coverage for golang is
|
|
Nice. |
|
Way to get 🥇 |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
Implement piped ReportStat handler in piped_api
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: