API limit remaining quota fixed #2048
Conversation
| if byHash { | ||
| limQuotaKey = QuotaKeyPrefix + id + "-" + sessionKey | ||
| } | ||
| if usedQuota, err := sessionManager.Store().GetRawKey(limQuotaKey); err == nil { |
There was a problem hiding this comment.
Makes sense to optimize this part by:
-
moving
storage.HashKey(sessionKey)out of loop(see line#366) -
[optional]moving apply quota logic to separate helper func in order to unify handling of sessionKey-quota & access-rights-quota with next pseudo-go-code
func applyQuota(quotaHandle interface{ ApplyQuota(int64) (int64, ?error) {
this.QuotaRemaining = ... // lines[371;377]
return this.QuotaRemaining, errorIfAny
} }, quotaStorageKey) error {
var usedQuota = ... //step1: get quotaFromStorage
quotaHandle.AppyQuota(usedQuota) //step2: apply quota and return error
}There was a problem hiding this comment.
looks like we are both replying to already merged PR :)
eventually there is lots of room to refactor here, the biggest piece I see is eliminating somehow repeated logic to populate usedQuota starting at
line https://github.com/TykTechnologies/tyk/pull/2048/files#diff-651a84f8ad6a38c1f7ccdf63f2692410R342
and
line https://github.com/TykTechnologies/tyk/pull/2048/files#diff-651a84f8ad6a38c1f7ccdf63f2692410R366
| continue | ||
| } | ||
|
|
||
| limQuotaKey := QuotaKeyPrefix + id + "-" + storage.HashKey(sessionKey) |
There was a problem hiding this comment.
quotaKey (to store quota on session level) and var limQuotaKey here (to store quota on session ACL level) - those keys have different format
There was a problem hiding this comment.
They share the same suffix part - storage.HashKey(sessionKey), which can be initialized at the beginning once.
| limitQuotaRemaining = currAccessRight.Limit.QuotaRemaining | ||
| limitQuotaRenews = currAccessRight.Limit.QuotaRenews | ||
| } | ||
| accessRights.Limit.QuotaRemaining = limitQuotaRemaining |
There was a problem hiding this comment.
- Isn't the changing of global policy from global
policiesByIDmap will mutate policy default value for the next request, is it? - Shouldn't there be default values for
limitQuotaRemaining&limitQuotaRenews? It seems it can override current(which is default if I'm correct) values for policy. Or is it for synchronization with values fetched from redis before inside this block?
@buger FYI
Feel free to confirm or disprove
| }, | ||
| }, | ||
| }...) | ||
| } |
added changes for TykTechnologies/tyk-analytics#987 The problem with quota-remaining is that it never makes to storage, we schedule session update in storage only when new renewal period starts, so we have to populate "remaining quota" counters directly from counters in redis when key gets requested via API. Also, added some tests for quota on API limit, something we should have done initially.
added changes for https://github.com/TykTechnologies/tyk-analytics/issues/987
The problem with quota-remaining is that it never makes to storage, we schedule session update in storage only when new renewal period starts, so we have to populate "remaining quota" counters directly from counters in redis when key gets requested via API.
Also, added some tests for quota on API limit, something we should have done initially.