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

[processor/transform] Add SpanID and TraceID to the grammar #10487

Merged

Conversation

TylerHelmuth
Copy link
Member

Description:
This PR adds the ability for the grammar to parse trace ids and span ids. With this change users can use trace/span ids in functions and conditions. Function creators will also be able to use SpanID and TraceID literals in their function's arguments.

Link to tracking Issue:
Originated from this PR/comment: #10471 (comment)

Testing:
Updated unit tests

Documentation:
Updated readme

@TylerHelmuth TylerHelmuth requested a review from a team June 1, 2022 15:47
@TylerHelmuth
Copy link
Member Author

/cc @anuraaga @bogdandrutu

processor/transformprocessor/internal/common/functions.go Outdated Show resolved Hide resolved
Comment on lines 183 to 184
{Name: `SpanIDWrapper`, Pattern: `{[a-fA-F0-9]{16}}`},
{Name: `TraceIDWrapper`, Pattern: `{[a-fA-F0-9]{32}}`},
Copy link
Member

Choose a reason for hiding this comment

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

What if I want to pass {0000000000000000} as a string instead of an id? How would I do that?

Copy link
Member Author

Choose a reason for hiding this comment

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

You would do "0000000000000000", but as part of #10471 we plan to make the trace and span accessor return the SpanID and TraceID structs, so any comparison or setting wouldn't work. This PR essentially says "If you want to interact with trace ids or span ids in this grammar you better wrap it in {}"

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about defining Bytes instead of specific span ID and trace ID types? It means that wrong lengths will not fail at parse time, but should still be failable when constructing the accessors in accessor validation. It could be useful elsewhere then.

Also it'd be good to think of some ideas on the syntax. {} feels a bit off personally, it looks like common template substitution paradigms. Couple of ideas

b'g00dbeef'
0xg00dbeef (perhaps literal can somehow have a helper to read this either as a int or a bytes)
hex("goodbeef")

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think about defining Bytes instead of specific span ID and trace ID types? It means that wrong lengths will not fail at parse time, but should still be failable when constructing the accessors in accessor validation. It could be useful elsewhere then.

I like a more generic Bytes idea, but how would that affect the TraceID and SpanID accessors? The original goal was to be able to interact with TraceID and SpanID directly and not need the accessors to convert to some other form. Although I think calling .SpanID().Bytes() and .TraceID().Bytes() is still an improvement from .HexString().

Would functions that want to work with a SpanID/TraceID need to instead accept a byte array and construct the ID themselves?

Also it'd be good to think of some ideas on the syntax. {} feels a bit off personally, it looks like common template substitution paradigms

I am definitely not sold on {}. In fact my original solution was SpanID{} and TraceID{} but I removed the identifiers.

I am against using () anywhere with this token since () feels reserved for functions. hex() would look like a function call when it is not.

0x is definitely the most hex-like, but it would feel weird when thrown in front of a trace or span id, only because that isn't how they are normally interacted with. People are most used to copy-pasting the ids everywhere as if they were strings. b'' kinda feels similar, but I like it the most of the three.

Maybe there is a combination solution? We can add Bytes to the lexer's token as b'[a-fA-F0-9]*', and add SpanID, TraceID, and Bytes to the Value struct like:

type Value struct {
	Invocation     *Invocation     `( @@`
	SpanIDWrapper  *SpanIDWrapper  `| "SpanID" "{" @Bytes "}"`
	TraceIDWrapper *TraceIDWrapper `| "TraceID" "{" @Bytes "}"`
	Bytes          *Bytes          `|  @Bytes`
	String         *string         `| @String`
	Float          *float64        `| @Float`
	Int            *int64          `| @Int`
	Bool           *Boolean        `| @("true" | "false")`
	Path           *Path           `| @@ )`
}

With this we could interact with a normal Byte[] like set(attributes["test"], b'010203') and interact with Span and Trace IDs like set(span_id, SpanID{b'0000000000000000'}) where trace_id == TraceID{b'00000000000000000000000000000000'}. Now it feels more like we are constructing a Span or Trace ID using a primitive type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe () would be good in that combined solution 🤔

Copy link
Member

Choose a reason for hiding this comment

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

SpanID(0x0123456789abcdef) feels appropriate to me. Treating SpanID(...) as a function that returns a SpanID struct. I would expect b'0101010101' to be a binary representation. As for what values would be appropriate to provide to a SpanID(...) function, I suspect that having a Bytes type and accepting that with a hex-literal format is correct, but it would probably also be good to have a function to convert hex strings to bytes and bytes to hex strings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should they be actual functions tho? I guess if the function constructs the struct and returns it in the actual ExprFunc that would be performant still.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm implementing a solution using 0x for a generic byte slice type and making new SpanID and TraceID functions, but it would be great if this PR was merged in first: #10489

Copy link
Member Author

Choose a reason for hiding this comment

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

NVM @djaglowski is a mind reader

@TylerHelmuth
Copy link
Member Author

The PR has been updated to handle a generic byte slice type using 0x and adds 2 new functions, TraceID and SpanID that take byte slices and return a SpanID and TraceID respectively.

Technically TraceID and SpanID do not fit the Telemetry Query Language's function syntax standards, but I didn't want to do create_TraceID or create_SpanID so I propose the function syntax standards be updated to allow "constructor" functions be allowed to not start with a verb.

@codeboten codeboten added the ready to merge Code review completed; ready to merge by maintainers label Jun 9, 2022
@codeboten
Copy link
Contributor

@mx-psi please review

@codeboten codeboten removed the ready to merge Code review completed; ready to merge by maintainers label Jun 9, 2022
@djaglowski djaglowski merged commit 5e799df into open-telemetry:main Jun 9, 2022
@TylerHelmuth TylerHelmuth deleted the issue-10350-update-grammar branch June 9, 2022 17:17
@mx-psi
Copy link
Member

mx-psi commented Jun 10, 2022

@mx-psi please review

Sorry, I had already clocked off for the day. This looks good to me in any case :)

kentquirk pushed a commit to McSick/opentelemetry-collector-contrib that referenced this pull request Jun 14, 2022
…emetry#10487)

* Add SpanID and TraceID to the grammar

* Updated NewGetter

* Updated readme

* Update NewFunctionCall to handle SpanID and TraceID

* Update Changelog

* Update processor/transformprocessor/internal/common/functions.go

Co-authored-by: Pablo Baeyens <[email protected]>

* Updated error message

* Fix lint issue

* Add byte slice type to grammar

* update tests

* Add TraceID function

* Add SpanID function

* Updated changelog

* Updated readme

* Add error check

* Fixing build checks

* Fix lint issues

Co-authored-by: Pablo Baeyens <[email protected]>
Co-authored-by: Daniel Jaglowski <[email protected]>
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.

6 participants