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] Add support for searching by trace:id and span:id #3670

Merged
merged 2 commits into from
May 23, 2024

Conversation

ie-pham
Copy link
Contributor

@ie-pham ie-pham commented May 13, 2024

What this PR does: Add support for searching by trace:id and span:id. One thing to note is that when comparing bytes array, Tempo will left trim "x00". This means that []byte{0x02} == []byte{0x00, 0x02}

Which issue(s) this PR fixes:
Fixes #
Screenshot 2024-05-13 at 5 47 16 PM

Checklist

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

@@ -999,7 +1000,7 @@ func createSpanIterator(makeIter makeIterFn, primaryIter parquetquery.Iterator,
switch cond.Attribute.Intrinsic {
case traceql.IntrinsicSpanID:

pred, err := createStringPredicate(cond.Op, cond.Operands)
pred, err := createBytesPredicate(cond.Op, cond.Operands, true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

placeholder until #3667 is merged with the new predicates

Copy link
Member

Choose a reason for hiding this comment

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

The new predicates will not have a bytes predicate. I would just write a new one in pkg/parquetquery/predicates.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the string predicate actually uses bytes array, reusing the string predicate works well. Any concerns about this?

Copy link
Member

Choose a reason for hiding this comment

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

nope. not if it does what we want

pkg/traceql/enum_attributes.go Show resolved Hide resolved
@@ -9,7 +9,7 @@ import (
"unsafe"
)

func HexStringToTraceID(id string) ([]byte, error) {
func HexStringToID(id string, isSpan bool) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The function HexStringToID() is no longer used outside this package (and probably should no longer be used, as it has merely become a helper function). Consider making the function private and maybe move it below the functions HexStringToTraceID() and HexStringToSpanID()

@stoewer
Copy link
Contributor

stoewer commented May 14, 2024

When I use an ID string that is too long when querying or span:id or trace:id (e.g. span:id="ffffffffffffffff0") I'm seeing the following error:

Query 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: creating span iterator: cannot convert hex to bytes array: ffffffffffffffff0

This is of course not wrong. But I'm wondering whether it would be better to just not return any matches

@ie-pham ie-pham changed the title [wip] [Traceql] Add support for searching by trace:id and span:id [Traceql] Add support for searching by trace:id and span:id May 20, 2024
@ie-pham ie-pham marked this pull request as ready for review May 20, 2024 23:03
Copy link
Contributor

@stoewer stoewer left a comment

Choose a reason for hiding this comment

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

LGTM
Awesome PR!

@ie-pham ie-pham merged commit 2e57b3a into grafana:main May 23, 2024
15 checks passed
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.

3 participants