-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
Added Caching of network interface for Azure #12622
Conversation
182b8a9
to
8a7c68e
Compare
Thanks for your contribution :-) I don't think the cache interval should be configurable. Additionally, I am not keen of importing a full caching library into Prometheus. I feel like a simple cache based on a map could do the trick. |
@roidelapluie thanks for taking the time to review, really appreciated. I understand your point on the lib to import, and yes using
First I was thinking of using So I found Concerning Of course if you are still not keen, I will just implement a simple LRU (inspired by the lib code). @roidelapluie let me know what you think. |
@roidelapluie Let me know what you think about my answer, I would like to move forward if possible. |
Thanks, we have discussed this during our bug scrub and we will go forward with this pull request. I will do a proper review later. |
thanks @roidelapluie |
@roidelapluie any luck in reviewing the PR ? |
Any news? I am desperately waiting for this change. |
Sorry to keep pushing you @roidelapluie |
discovery/azure/azure.go
Outdated
if d.cache == nil { | ||
return | ||
} | ||
rand.Seed(time.Now().UnixNano()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you use random here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I didn't want the key to expire at the same time, which will cause the same effect as having no cache.
If Key expire at the same time we will just consume all the api call that we try to avoid with the cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree, if the cache lifetime is >
than the refresh interval, the cache is worth it. I do not see why we need to introduce random sleep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of introducing a random bias to the expiration time to mitigate spikes when the initial batches of nodes expiring is quite reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have remove the seed put let the random [0-refreshInterval] added to the exp time.
27efa27
to
8a4cb71
Compare
discovery/azure/azure.go
Outdated
if d.cache == nil { | ||
return | ||
} | ||
random := time.Duration(d.cfg.RefreshInterval) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you want random instead of 10* refresh interval?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I guess I am missing something:
We don't want the item to expire at the same time, because if that happen will hit the api call limit, everytime the cache expire, so to mitigate that I use a random to avoid everything to expire at the same time (within a few sec).
Signed-off-by: Etourneau Gwenn <getourneau@yugabyte.com>
Signed-off-by: Etourneau Gwenn <getourneau@yugabyte.com>
Signed-off-by: Etourneau Gwenn <getourneau@yugabyte.com>
Enabled cache by default with 5x the default refresh time Signed-off-by: Etourneau Gwenn <getourneau@yugabyte.com>
Signed-off-by: Etourneau Gwenn <getourneau@yugabyte.com>
Signed-off-by: Etourneau Gwenn <getourneau@yugabyte.com>
Signed-off-by: Etourneau Gwenn <getourneau@yugabyte.com>
47b5b6b
to
36a5843
Compare
Removed uneeded error Signed-off-by: Etourneau Gwenn <getourneau@yugabyte.com>
@roidelapluie anything you need from my side ? thanks |
…hing Signed-off-by: Etourneau Gwenn <getourneau@yugabyte.com>
@roidelapluie I have rebase from master, run the test again, all good |
small bump |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(During bug scrub).
LGTM! We were looking if we could reuse some existing caches, but we only have k8s or hard coded ones. Plus the https://github.com/Code-Hex/go-generics-cache/blob/main/go.mod seems pretty slim - it's fine for now.
This PR add the caching of Network API interface for Azure this is related to issue #12613 .
This PR will:
cache_refresh_interval
which is a part of the calculation of final cache refreshingprometheus_sd_azure_cache_hit_total
Cache is disabled by default that means same behavior as without the PR.
The cache itself as an hard limit to 10K items, the refresh (expiration time) is by item and calculated like
cache_refresh_interval + refresh_interval + random(20s)).
We add
refresh_interval
because the cache have no utility if we cannot save item for at least onerefresh_interval
We add
random(20s)
to avoid that all object expired within a small amount of time.Have been tested :
-race