Skip to content

Tracing: enable tracing for main query execution steps #544

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pedro-stanaka
Copy link
Contributor

@pedro-stanaka pedro-stanaka commented Mar 26, 2025

Summary

Adding tracing to some steps inside the engine so we can get better visibility into where we spend time. Had to duplicate some code from Thanos to avoid cyclic dependency.

This is what a trace will look like:

image

@pedro-stanaka pedro-stanaka force-pushed the feat/tracing-improvements branch 5 times, most recently from b9faa50 to 393e6f9 Compare March 28, 2025 08:48
@pedro-stanaka pedro-stanaka marked this pull request as ready for review March 28, 2025 11:17
@pedro-stanaka pedro-stanaka force-pushed the feat/tracing-improvements branch 2 times, most recently from 6bfdf1c to a595f5c Compare March 31, 2025 08:00
Signed-off-by: Pedro Tanaka <[email protected]>

fixing go mods and docs

Signed-off-by: Pedro Tanaka <[email protected]>

move docs to go package

Signed-off-by: Pedro Tanaka <[email protected]>
@pedro-stanaka pedro-stanaka force-pushed the feat/tracing-improvements branch from 9155080 to 0fd592d Compare March 31, 2025 08:08
Signed-off-by: Pedro Tanaka <[email protected]>

appease linter

Signed-off-by: Pedro Tanaka <[email protected]>
@pedro-stanaka pedro-stanaka force-pushed the feat/tracing-improvements branch from 0fd592d to 708a108 Compare March 31, 2025 08:09
Copy link
Contributor

@harry671003 harry671003 left a comment

Choose a reason for hiding this comment

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

Thanks for the great work! Left some minor comments.

As a next step, should the time taken in each operator be measured?

@@ -237,14 +248,21 @@ type Engine struct {
}

func (e *Engine) MakeInstantQuery(ctx context.Context, q storage.Queryable, opts *QueryOpts, qs string, ts time.Time) (promql.Query, error) {
span, ctx := tracing.StartSpanFromContext(ctx, "engine.MakeInstantQuery")
defer span.Finish()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we expect the engine to take a lot of time before query execution phase? Does these spans add value?

defer span.Finish()
span.SetTag("query_type", q.t.String())
span.SetTag("query_string", q.String())
if q.t == RangeQuery {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Should we just set start=end and step=0 for instant query?

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