Skip to content

[TraceQL] Autocomplete for events and links#3846

Merged
ie-pham merged 8 commits intografana:mainfrom
ie-pham:autocomplete
Jul 15, 2024
Merged

[TraceQL] Autocomplete for events and links#3846
ie-pham merged 8 commits intografana:mainfrom
ie-pham:autocomplete

Conversation

@ie-pham
Copy link
Copy Markdown
Contributor

@ie-pham ie-pham commented Jul 4, 2024

What this PR does:
Add autocomplete support for events and links. Return empty responses for IDs in autocomplete.
Also returns event:name, link:spanID, and link:traceID to search results to be displayed on the UI

Grafana UI is not updated yet but API works as expected
Screenshot 2024-07-04 at 5 05 02 PM
Screenshot 2024-07-04 at 5 05 22 PM
Screenshot 2024-07-04 at 5 28 04 PM

event:name, link:spanID, and link:traceID
Screenshot 2024-07-01 at 9 37 26 PM
Screenshot 2024-07-01 at 9 38 01 PM

Which issue(s) this PR fixes:
Fixes #

Checklist

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

@ie-pham ie-pham changed the title [wip] [TraceQL] Autocomplete for events and links [TraceQL] Autocomplete for events and links Jul 9, 2024
@ie-pham
Copy link
Copy Markdown
Contributor Author

ie-pham commented Jul 9, 2024

One caveat is because links and events are on the same definition level, they cannot be used to filter each other out effectively. I prefer to get the main functionality out first and create a new ticket to add this ability.

Comment thread integration/e2e/multi_tenant_test.go Outdated
tagsV2Resp, err := apiClient.SearchTagsV2()
require.NoError(t, err)
require.Equal(t, 3, len(tagsV2Resp.GetScopes()))
fmt.Println(tagsV2Resp.Scopes)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Was this print statement for debugging or is it meant to stay?

"github.com/grafana/tempo/pkg/model/trace"
"github.com/grafana/tempo/pkg/tempopb"
v1 "github.com/grafana/tempo/pkg/tempopb/common/v1"
v11 "github.com/grafana/tempo/pkg/tempopb/trace/v1"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick we often use 'trace_v1', 'resource_v1` as import aliases

Suggested change
v11 "github.com/grafana/tempo/pkg/tempopb/trace/v1"
trace_v1 "github.com/grafana/tempo/pkg/tempopb/trace/v1"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ahhh I see, it's v11 in the pb.go files

Comment on lines +176 to +178
// if len(iters) == 0 && primaryIter == nil {
// return attrIter, nil
// }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this meant to be commented out? Just asking because the equivalent condition in createDistinctLinkIterator is not commented out (see L236)

@stoewer
Copy link
Copy Markdown
Contributor

stoewer commented Jul 10, 2024

One caveat is because links and events are on the same definition level, they cannot be used to filter each other out effectively. I prefer to get the main functionality out first and create a new ticket to add this ability

I think it's a good idea to the the main functionality out and handle edge cases in another issue

@stoewer
Copy link
Copy Markdown
Contributor

stoewer commented Jul 10, 2024

Tested it locally and it works as expected. Just left some minor comments

Comment thread modules/ingester/instance_search.go Outdated
Comment thread tempodb/encoding/vparquet4/block_autocomplete.go Outdated
Comment thread tempodb/encoding/vparquet4/block_autocomplete.go Outdated
@ie-pham ie-pham requested a review from stoewer July 15, 2024 14:45
Copy link
Copy Markdown
Contributor

@mdisibio mdisibio left a comment

Choose a reason for hiding this comment

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

Events tested locally and working well. I don't have a good testbed for links, but lgtm.

@knylander-grafana
Copy link
Copy Markdown
Contributor

Will we need docs for this?

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.

4 participants