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

feat: limits #559

Merged
merged 27 commits into from
Jun 7, 2024
Merged

feat: limits #559

merged 27 commits into from
Jun 7, 2024

Conversation

grutt
Copy link
Contributor

@grutt grutt commented Jun 5, 2024

Description

This PR introduces a new feature to impose and manage tenant-specific limits within our system. The key metrics being tracked include task executions per day, concurrent workers (burstable), and users. The feature includes mechanisms for alerting and blocking based on predefined usage thresholds.

Type of change

  • New feature (non-breaking change which adds functionality)

What's Changed

  • Add db models for tracking tenant resource limits
  • Add entitlement checks and metering queries to relevant requests
  • Expose Tenant Resource Limit API
  • Add new Tenant Resource Limit Alert Ticker

Copy link

vercel bot commented Jun 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hatchet-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 7, 2024 3:48pm

@grutt grutt marked this pull request as ready for review June 6, 2024 14:57
@grutt grutt requested a review from abelanger5 June 6, 2024 14:57
return limits, nil
}

func (t *tenantLimitRepository) CanCreate(ctx context.Context, resource dbsqlc.LimitResource, tenantId string) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to use the cache here, it's not ideal that every single lookup to determine whether to create an event/workflow run involves a database lookup, part of the design here would be to reduce the database load. I'd suggest at least caching for a minute here.

@@ -40,6 +40,10 @@ type EngineRepository interface {
RateLimit() RateLimitEngineRepository
}

type EntitlementsRepository interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have the EntitlementsRepository? Why not have the TenantLimitRepository as a direct call in the ApiRepository and EngineRepository which also gets passed as an argument to the metering struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured it make more sense to separate this responsibility and share it across the two repositories. in the future we'll likely want to add additional things on the entitlement repositories (i.e. feature flags) so i think having it as its own thing make sense

if err == pgx.ErrNoRows {
t.l.Warn().Msgf("no %s tenant limit found, creating default limit", string(resource))

err = t.CreateTenantDefaultLimits(ctx, tenantId)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify, this means that all existing tenants will get a limit set to the default?

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, on first meter it'll insert if none exists

@grutt grutt merged commit bbc4e58 into main Jun 7, 2024
20 checks passed
@grutt grutt deleted the feat-enforced-limits branch June 7, 2024 17:57
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.

2 participants