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

promquery: add BulkQueryCache #100

Merged
merged 1 commit into from
Jan 3, 2024
Merged

Conversation

majewsky
Copy link
Contributor

@majewsky majewsky commented Dec 20, 2023

This implementation originates in Limes (imported from there at internal/util/promcache.go), but I want to reuse it in Castellum where the idea of bulk requesting and caching Prometheus metrics originates.

This implementation differs slightly from the one in Limes:

  • The Prometheus prefix was removed from the type names to fit the package name.
  • The refreshRate field was renamed to refreshInterval ("rate" would be the inverse of an interval).
  • The constructor method takes a Client instance instead of its Config to avoid double initialization work in the general case.
  • A full-featured documentation example was added to better demonstrate how all the pieces fit together.

Note: Because of the year-end change freeze, please do not merge this yet. Once approved, I will merge after the end of the change freeze.

This implementation originates in Limes (imported from there at
`internal/util/promcache.go`), but I want to reuse it in Castellum where
the idea of bulk requesting and caching Prometheus metrics originates.

This implementation differs slightly from the one in Limes:

- The `Prometheus` prefix was removed from the type names.
- The `refreshRate` field was renamed to `refreshInterval`.
- The constructor method takes a Client instance instead of its Config.
- A full-featured documentation example was added to better demonstrate
  how all the pieces fit together.
// Get returns the entry for this key, or a zero-initialized entry if this key
// does not exist in the dataset.
func (c *BulkQueryCache[K, V]) Get(key K) (entry V, err error) {
err = c.fillCacheIfNecessary()
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this cause spikes in the execution time of Get() whenever the interval is past? So we shouldn't use this in any time critical functions like http requests which are supposed to return fast.

All the alternative ideas I have cause constant background refreshes even if things are idle or the first request would always be cashed even if the interval is past.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but we are initially only using this for background tasks (in limes-collector and castellum-observer), so this performance characteristic is not a problem.

If we want to use this for frontend tasks later on, the design affords adding explicit refreshes ahead of schedule that bypass the existing "if necessary" check. This would be similar to what we do in swift-s3-credential-prewarmer, except at a different scale (within a single process instead of within an entire service).

@majewsky majewsky marked this pull request as ready for review January 3, 2024 13:12
@SuperSandro2000 SuperSandro2000 merged commit 2e41a6b into master Jan 3, 2024
6 checks passed
@SuperSandro2000 SuperSandro2000 deleted the promquery-bulkcache branch January 3, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants