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

TraceQL event intrinsic event:name #3708

Merged
merged 8 commits into from
May 28, 2024

Conversation

stoewer
Copy link
Contributor

@stoewer stoewer commented May 22, 2024

What this PR does:

Add event scope and the intrinsic event:name

Autocomplete is currently not working, but is also part of a separate issue.

Which issue(s) this PR fixes:
Contributes to grafana/tempo-squad#424

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@stoewer stoewer changed the title Traceql additions event intrinsics [wip] TraceQL event intrinsic event:name May 24, 2024
@stoewer
Copy link
Contributor Author

stoewer commented May 24, 2024

Local tests with docker-compose examples

image

@stoewer
Copy link
Contributor Author

stoewer commented May 24, 2024

There are two behaviors that I'm not quite sure about. It would be great to get feedback about them

  1. The query { event:name != "exception" } also returns spans without any events at all. Is this correct or should we only return spans with events that don't have the specified name?
  2. When running a with query:name on vParquet2 or vParquet3 it shows the following error: "rpc error: code = Internal desc = error querying ingesters in Querier.Search: failed to execute f() for 127.0.0.1:9095: rpc error: code = Unknown desc = creating fetch iter: error creating iterator: unsupported traceql scope: event:name". Do we want to show this error, or should we rather just show no results? see here

@stoewer stoewer force-pushed the traceql-additions-event-intrinsics branch from ba63592 to 3d529e0 Compare May 24, 2024 03:35
@stoewer stoewer marked this pull request as ready for review May 24, 2024 03:35
@@ -833,6 +834,7 @@ var intrinsicColumnLookups = map[traceql.Intrinsic]struct {
traceql.IntrinsicTraceID: {intrinsicScopeTrace, traceql.TypeString, columnPathTraceID},
traceql.IntrinsicTraceStartTime: {intrinsicScopeTrace, traceql.TypeDuration, columnPathStartTimeUnixNano},

traceql.IntrinsicEventName: {intrinsicScopeEvent, traceql.TypeNil, ""}, // Not used in vparquet3, this entry is only used to assign default scope.
Copy link
Contributor

@mdisibio mdisibio May 24, 2024

Choose a reason for hiding this comment

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

Since this isn't supported in vParquet2/3, I think we need to ensure that a query using event intrinsics doesn't execute. Currently, it looks like the condition will be silently dropped while fetching, which means the query results could be incorrect: the {name=x && event:name=y} will be executed against a vp2/3 block as {name=x}. checkConditions could return ErrUnsupported if any event scope condition is present. Didn't see if this is caught elsewhere.

Copy link
Contributor Author

@stoewer stoewer May 27, 2024

Choose a reason for hiding this comment

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

Currently queries with event:name fail in categorizeConditions() which results in a rather cryptic error reported in the UI. I agree, it's much better to check this explicitly in checkConditions() and wrap ErrUnsupported.

eventIters = append(eventIters, primaryIter)
}

return parquetquery.NewJoinIterator(4, eventIters, &eventCollector{}, parquetquery.WithPool(pqEventPool)), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: let's create a const for the definitionlevel=4

@stoewer stoewer force-pushed the traceql-additions-event-intrinsics branch from 63edfbf to 7bb6ef4 Compare May 27, 2024 04:57
@ie-pham
Copy link
Contributor

ie-pham commented May 27, 2024

LGTM! Awesome work! Will steal this for my PR 😄

Nit: May I suggest a test in block_traceql_meta_test.go ?

{
			"Event name lookup",
			makeReq(
				parse(t, `{event:name = "e1"}`), // 
			),
			makeSpansets(
				makeSpanset(
					wantTr.TraceID,
					wantTr.RootSpanName,
					wantTr.RootServiceName,
					wantTr.StartTimeUnixNano,
					wantTr.DurationNano,
					&span{
						id:                 wantTr.ResourceSpans[0].ScopeSpans[0].Spans[0].SpanID,
						startTimeUnixNanos: wantTr.ResourceSpans[0].ScopeSpans[0].Spans[0].StartTimeUnixNano,
						durationNanos:      wantTr.ResourceSpans[0].ScopeSpans[0].Spans[0].DurationNano,
						spanAttrs: []attrVal{
							{traceql.NewIntrinsic(traceql.IntrinsicDuration), traceql.NewStaticDuration(100 * time.Second)},
							{traceql.NewIntrinsic(traceql.IntrinsicSpanID), traceql.NewStaticString(util.SpanIDToHexString(wantTr.ResourceSpans[0].ScopeSpans[0].Spans[0].SpanID))},
						},
						traceAttrs: []attrVal{
							{traceql.NewIntrinsic(traceql.IntrinsicTraceRootService), traceql.NewStaticString("RootService")},
							{traceql.NewIntrinsic(traceql.IntrinsicTraceRootSpan), traceql.NewStaticString("RootSpan")},
							{traceql.NewIntrinsic(traceql.IntrinsicTraceDuration), traceql.NewStaticDuration(100 * time.Millisecond)},
							{traceql.NewIntrinsic(traceql.IntrinsicTraceID), traceql.NewStaticString(util.TraceIDToHexString(wantTr.TraceID))},
						},
						eventAttrs: []attrVal{
							{traceql.NewIntrinsic(traceql.IntrinsicEventName), traceql.NewStaticString("e1")},
						},
					},
				),
			),
		},

@stoewer
Copy link
Contributor Author

stoewer commented May 27, 2024

May I suggest a test in block_traceql_meta_test.go?

Good point, thank you. I've added the test case

@stoewer stoewer merged commit a566550 into grafana:main May 28, 2024
14 checks passed
@stoewer stoewer deleted the traceql-additions-event-intrinsics branch May 28, 2024 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants