metrics: use contexts #9949
Open
metrics: use contexts #9949
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robert-uhl
requested changes
Apr 12, 2024
| @@ -146,27 +146,31 @@ func FinishReportAndFlushUserAction(action string, err error, start time.Time) f | |||
| return wait | |||
| } | |||
|
|
|||
| func (r *Reporter) reportClusterMetrics(ctx context.Context) { | |||
| func (r *Reporter) reportClusterMetrics(rctx context.Context) { | |||
There was a problem hiding this comment.
There’s no need for *r*ctx here.
| return | ||
| case <-ticker.C: | ||
| } | ||
| metrics := &Metrics{} | ||
| r.internalMetrics(metrics) | ||
| externalMetrics(r.env.GetKubeClient(), metrics) //nolint:errcheck | ||
| ctx, c := context.WithTimeout(rctx, reportingInterval/2) |
There was a problem hiding this comment.
If all this is put into the ticker case of the select (which I think makes more sense anyway), the the scoping here is more obvious.
| r.internalMetrics(metrics) | ||
| externalMetrics(r.env.GetKubeClient(), metrics) //nolint:errcheck | ||
| ctx, c := context.WithTimeout(rctx, reportingInterval/2) | ||
| r.internalMetrics(ctx, metrics) |
There was a problem hiding this comment.
Why dedicate half the time to collecting and half to reporting, instead of trying to do both in total? time.Ticker “will adjust the time interval or drop ticks to make up for slow receivers.”
var (
metrics = new(Metrics)
ctx, cancel = context.WithTimeout(ctx, reportingInterval)
)
defer cancel()
r.internalMetrics(ctx, metrics)
externalMetrics(ctx, r.env.GetKubeClient(), metrics) //nolint:errcheck
metrics.ClusterId = r.clusterID
metrics.PodId = uuid.NewWithoutDashes()
metrics.Version = version.PrettyPrintVersion(version.Version)
r.router.reportClusterMetricsToSegment(metrics)Edit: deleted mistake.
| @@ -248,12 +252,9 @@ func inputMetrics(input *pps.Input, metrics *Metrics) { | |||
| } | |||
| } | |||
|
|
|||
| func (r *Reporter) internalMetrics(metrics *Metrics) { | |||
| func (r *Reporter) internalMetrics(ctx context.Context, metrics *Metrics) { | |||
There was a problem hiding this comment.
There is a call to pctx.TODO in here as well (and a bonus typo!).
There was a problem hiding this comment.
Also, the TTL and context lifetime differential below is a bit surprising.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This avoids a dpanic in the grpc client logger while collecting internal metrics. It also puts a limit on how long metrics will be collected for.
I don't know if we even get these metrics anymore, but at least pachd doesn't die when PACHYDERM_DEVELOPMENT_LOGGER=1.