-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
refactor(blooms): Add metrics for per-tenant tasks progress to planner #13078
refactor(blooms): Add metrics for per-tenant tasks progress to planner #13078
Conversation
e99a459
to
a0c5a5e
Compare
pkg/bloombuild/planner/planner.go
Outdated
@@ -149,6 +151,8 @@ func (p *Planner) running(ctx context.Context) error { | |||
} | |||
|
|||
case <-inflightTasksTicker.C: | |||
// TODO(salvacorts): Is think this won't evaluate while the runOne is running. | |||
// We should run the runOne in a goroutine and check the context inside (as we already do). |
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.
nit: I would rather run the ticker in a new goroutine
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.
Thank you! Done
p.metrics.tenantTasksPlanned.WithLabelValues(tenant).Set(0) | ||
p.metrics.tenantTasksCompleted.WithLabelValues(tenant).Set(0) |
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.
nit: should we remove the metric with the tenant label completely, instead of just resetting it to 0?
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.
Also, we could then use a Counter
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.
should we remove the metric with the tenant label completely, instead of just resetting it to 0?
AFAICT a metric cannot be explicitly removed. Can it?
we could then use a Counter
That was my first idea but a counter would make it a bit difficult to see for a given iteration, how many "new" tasks would be created. It would require using increase
but setting the interval is difficult. Instead, by using a gauge, you can see the current set of tasks.
Also note that the gauge doesn't change often, but only when the build is planned.
What this PR does / why we need it:
This PR adds two new metrics to track the build process of each tenant.
Special notes for your reviewer:
loki/pkg/queue/metrics.go
Lines 16 to 21 in 5b8d0e6
Checklist
CONTRIBUTING.md
guide (required)feat
PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.md
production/helm/loki/Chart.yaml
and updateproduction/helm/loki/CHANGELOG.md
andproduction/helm/loki/README.md
. Example PRdeprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR