[TraceQL] Add support for querying by instrumentation scope #3967
Conversation
ie-pham
requested review from
knylander-grafana,
joe-elliott,
annanay25,
mdisibio,
mapno,
yvrhdn,
zalegrala,
electron0zero and
stoewer
as code owners
|
cc @knylander-grafana possibly merged before 2.6 and would be a big feature |
docs/sources/tempo/traceql/_index.md
Outdated
| @@ -94,6 +94,8 @@ The following table shows the current available scoped intrinsic fields: | |||
| | `event:timeSinceStart` | duration | time of event in relation to the span start time | `{ event:timeSinceStart > 2ms}` | | |||
| | `link:spanID` | string | link span id using hex string | `{ link:spanID = "0000000000000001" }` | | |||
| | `link:traceID` | string | link trace id using hex string | `{ link:traceID = "1234567890abcde" }` | | |||
| | `scope:name` | string | instrumentation scope name | `{ scope:name = "grpc" }` | | |||
There was a problem hiding this comment.
I thought the thing at the left of : was a scope , i.e. link is a possible value for scope.
So is very confusing for me that this is called scope , shouldn't it be called instrumentation ?
instrumentation:name instrumentation:version more clearly define what this points at IMHO
There was a problem hiding this comment.
this is a good call out. we have discussed this with our internal team and they believe that otel does not see this as an instrumentation only field. it just happens to be used that way and they intend for it to be used as a more generic "scope" long term.
agree that scope scope is strange. let me summon @jpkrohling. he may have more insight.
There was a problem hiding this comment.
Are end-users exposed to "scope scope"? As an OTel user, I immediately thought of InstrumentationScope when I saw "scope:name" here. Unless there's an explicit source of confusion, I'd keep "scope:name".
There was a problem hiding this comment.
Are end-users exposed to "scope scope"?
Not directly although there may be some awkward wording in the docs.
There was a problem hiding this comment.
Personally scope:name is a generic term that says nothing to me.
I can imagine for people better versed in opentelemetry parlance it is very obvious.
I am very sure that around here I will need to tell people that "scope:name means the name of the instrumentation/area that created the span". or such explanation as I do not think it is clear to more casual users of observability tools.
There was a problem hiding this comment.
Would it help to have these differences called out in the doc to avoid confusion? Or would that just create more confusion?
There was a problem hiding this comment.
Docs will definitely need to be there.
When someone reads a query or an autocomplete with span:duration , that person needs to know almost nothing about tracing to understand what that would mean, without resorting to reading documentation.
I think when someone reads scope:name that person will definitely need to go to documentation to know what would that mean.
There was a problem hiding this comment.
Other than the number of keyboard touches, I have no strong preferences for scope vs. instrumentation. They are both technically correct. If instrumentation is more intuitive to the audience, that certainly wins over scope.
There was a problem hiding this comment.
@jcarres-mdsol thank you for pointing this out. I've changed the syntax to instrumentation:name and instrumentation:version 🚀
docs/sources/tempo/traceql/_index.md
Outdated
| @@ -94,6 +94,8 @@ The following table shows the current available scoped intrinsic fields: | |||
| | `event:timeSinceStart` | duration | time of event in relation to the span start time | `{ event:timeSinceStart > 2ms}` | | |||
| | `link:spanID` | string | link span id using hex string | `{ link:spanID = "0000000000000001" }` | | |||
| | `link:traceID` | string | link trace id using hex string | `{ link:traceID = "1234567890abcde" }` | | |||
| | `scope:name` | string | instrumentation scope name | `{ scope:name = "grpc" }` | | |||
| | `scope:version` | string | instrumentation scope version | `{ scope:version = "1.0.0.0" }` | | |||
There was a problem hiding this comment.
Was one .0 accidentally added?
| | `scope:version` | string | instrumentation scope version | `{ scope:version = "1.0.0.0" }` | | |
| | `scope:version` | string | instrumentation scope version | `{ scope:version = "1.0.0" }` | |
There was a problem hiding this comment.
Running locally things are looking good, searching, autocomplete, and selectall via compare() (nice!). Approving ✅
But did want to call out some unexpected behavior, that this PR is not causing, but unearthing. If a span is sent with no instrumentation, we save name/value to parquet as empty strings, and they are maintained for reads. Empty string is returned from autocomplete /api/v2/search/tag/instrumentation:name/values and /api/v2/search/tag/instrumentation:version/values, and the query {instrumentation:name="" && instrumentation:version=""} finds these spans.
But I think we got lucky. Although this behavior is unexpected, it's technically correct. The otel proto definition is commented Semantically when InstrumentationScope isn't set, it is equivalent with an empty instrumentation scope name (unknown). Now it's sounding familiar and we likely considered this when designing vParquet1 schema to be non-nullable (which this inherited).
Either way, I think we are good and just wanted to mention it for future reference.
What this PR does: Add TraceQL to search by instrumentation scope name, version, and attributes.
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]