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

Compactor: Add tracing support #4903

Merged
merged 23 commits into from
Dec 1, 2021
Merged

Conversation

metonymic-smokey
Copy link
Contributor

@metonymic-smokey metonymic-smokey commented Nov 25, 2021

Fixes #4832

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Added tracing support for the various compaction steps(similar to Querier).

Verification

Tested locally using Jaeger.

metonymic-smokey and others added 16 commits November 18, 2021 18:36
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

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

This is looking cool! I was wondering about one more thing, which I think could be particularly useful in the context of compaction and that is to also record the error in the span (we would have the error in the logs, but I think having them directly on span would be more handy).

We are using our convenience method DoInSpan here, but that does not capture an error if it occurs. Maybe we could have another method (e.g. DoInSpanWithErr) which could also set the error on span. Does that sound good?

pkg/compact/compact.go Outdated Show resolved Hide resolved
Co-authored-by: Matej Gera <38492574+matej-g@users.noreply.github.com>
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

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

Great job @metonymic-smokey! Thanks for going above and beyond to address things. I'm wondering about one last thing, afterwards I believe this is ready.

Let's try to get a review from maintainer as well, perhaps @yeya24 as original issue reporter.

span, newCtx := StartSpan(ctx, operationName, opts...)
defer span.Finish()
err := doFn(newCtx)
span.LogFields(opentracing_log.Error(err))
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. I'm not sure about the behavior if err is nil (whether it still logs error), but maybe we could check err != nil before we log error?
  2. I think we can replace this with method from the ext package, see https://pkg.go.dev/github.com/opentracing/opentracing-go@v1.2.0/ext#LogError. This will set all necessary things on the span for us

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point @matej-g. Thanks!
I've made the changes in a recent commit. Let me know if they're okay.

Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
matej-g
matej-g previously approved these changes Dec 1, 2021
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

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

💯

defer span.Finish()
err := doFn(newCtx)
if err != nil {
ext.LogError(span, err, opentracing_log.Error(err))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we do not need to pass the last value, since the method will already include the field for error.

squat
squat previously approved these changes Dec 1, 2021
Copy link
Member

@squat squat left a comment

Choose a reason for hiding this comment

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

Looks good overall! Some very small nits

err = block.Download(ctx, cg.logger, cg.bkt, meta.ULID, bdir)
return err
}, opentracing.Tags{"block.id": meta.ULID})

Copy link
Member

Choose a reason for hiding this comment

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

tiny nit

Suggested change

return false, ulid.ULID{}, retry(errors.Wrapf(err, "download block %s", meta.ULID))
}

// Ensure all input blocks are valid.
stats, err := block.GatherIndexHealthStats(cg.logger, filepath.Join(bdir, block.IndexFilename), meta.MinTime, meta.MaxTime)
var stats block.HealthStats
tracing.DoInSpanWithErr(ctx, "compaction_block_healthstats", func(ctx context.Context) error {
Copy link
Member

Choose a reason for hiding this comment

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

Since the Golang name is ...HealthStats, health and stats should be treated as separate words

Suggested change
tracing.DoInSpanWithErr(ctx, "compaction_block_healthstats", func(ctx context.Context) error {
tracing.DoInSpanWithErr(ctx, "compaction_block_health_stats", func(ctx context.Context) error {

@@ -764,7 +766,11 @@ func (cg *Group) Compact(ctx context.Context, dir string, planner Planner, comp
return false, ulid.ULID{}, errors.Wrap(err, "create compaction group dir")
}

shouldRerun, compID, err := cg.compact(ctx, subDir, planner, comp)
var err error
tracing.DoInSpanWithErr(ctx, "group_compaction", func(ctx context.Context) error {
Copy link
Member

Choose a reason for hiding this comment

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

All other spans are named compaction_<x>, should we make this follow the same convention?

Signed-off-by: metonymic-smokey <ahuja.aditi@gmail.com>
@metonymic-smokey metonymic-smokey dismissed stale reviews from squat and matej-g via 01812fc December 1, 2021 12:11
@squat
Copy link
Member

squat commented Dec 1, 2021

lgtm! restarting failed jobs and we can merge :))

@squat squat enabled auto-merge (squash) December 1, 2021 12:39
@metonymic-smokey
Copy link
Contributor Author

Thanks for the reviews @matej-g @squat !

@squat squat merged commit d08a12a into thanos-io:main Dec 1, 2021
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.

Add tracing support for compaction
3 participants