-
Notifications
You must be signed in to change notification settings - Fork 530
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
Design Doc: TraceQL Extensions #3185
Conversation
Signed-off-by: Joe Elliott <[email protected]>
|
||
### Instrumentation Scope | ||
|
||
The [Instrumentation Scope](https://github.com/open-telemetry/opentelemetry-proto/blob/ea449ae0e9b282f96ec12a09e796dbb3d390ed4f/opentelemetry/proto/common/v1/common.proto#L71) object contains information about the instrumentation used to generate telemetry data. This scope contains new intrinsics as well as attributes. It will be referenced using the `scope` keyword. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned with overusing the word scope
. We already use it as a concept in TraceQL (ie. {span,resource}.foo
, and now it's a type of scope. Scope scope
.
We could use instrumentation
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered instrumentation
, but it's so long! Since the name/version/attributes reflect the instrumentation we should probably name it something along those lines. instrumentation
is clearer but unwieldy, but this will be rarely typed w/o auto complete so maybe it's ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but this will be rarely typed w/o auto complete so maybe it's ok?
Right, rarely a user would have to type the whole thing.
Scopes in general are a concept that many users already struggle with. Introducing the scope scope
will only add to the confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, besides the name collision, instrumentation
is clearer in what it refers to. scope
could be anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OTEL docs and concepts will still refer to it as the instrumentation scope. I have to use instrumentation_scope.name
in ottl filters in the otel-collector for some of our endpoints for example. New users come across references to the instrumentation scope before they start digging around in the protos to find the Scope[Spans|Metrics|Logs]
messages in my experience since the SDK hides the proto details from them. For better or worse, the docs inform how the users think about it more than the actual proto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lmickh what would you say you are in favor of? we're discussing this internally with our otel folks and they have pointed out that "instrumentation scope" is more closely aligned with a scope ("logical unit of code") than with instrumentation. i'm a bit torn on the choice myself.
Co-authored-by: Mario <[email protected]>
|
||
TraceQL will also support the escape sequences `\\` and `\"` in an attribute name. | ||
|
||
`{ span."this is \"bad\"" = "foo"}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding support for single quotes too, so users don't have to deal with escaping. For reference there is a plan to do that in prometheus to address full utf-8 support. Proposal link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the heads up. i like the way they've used both "
and '
for their quoted attributes. it's something we should definitely keep in mind for a future improvement.
This PR adds a design doc aimed at the next set of functionality for the TraceQL language. We are looking for community feedback, consensus and comments before merging and beginning this work.
Please jump in and comment!