Repository navigation
lib/traceql: support unscoped attributes in TraceQL queries - #264
niladrix719 wants to merge 1 commit into
Conversation
Signed-off-by: Niladri Adhikary <niladrix719@gmail.com>
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/traceql/filter_general.go">
<violation number="1" location="lib/traceql/filter_general.go:116">
P3: Dotted intrinsic names (`.name`, `.kind`, `.status`, `.duration`) now fall into this unscoped-attribute branch and are resolved as `span_attr:`/`resource_attr:` attribute lookups, while the same keywords without the dot resolve to the span name stream field (`name`), `status_code`, `kind`, or the `duration` field. `.status = "error"` will never match the status intrinsic, and a span whose literal attribute is named `status` would now match where it previously didn't. Extend the special-case beyond `.service.name` (or remap these dotted forms to the intrinsic fields) so unscoped intrinsics keep intrinsic semantics.</violation>
<violation number="2" location="lib/traceql/filter_general.go:120">
P3: For nil-value ops the generated expression contains a dead sub-filter: `{.foo != nil}` emits `("span_attr:foo":* and "span_attr:foo":* or ...)` (the presence guard is duplicated) and `{.foo = nil}` emits `("span_attr:foo":* and "span_attr:foo":"" or ...)`, where the first `and` branch is never satisfiable and the second branch already requires `"span_attr:foo":""`. Emit the span filter without re-adding the guard when the filter already encodes presence/absence (nil ops), e.g. `"span_attr:foo":"" and "resource_attr:foo":""` for `= nil`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| spanFilter := filterCommon{fieldName: "span." + attr, op: fc.op, value: fc.value} | ||
| resourceFilter := filterCommon{fieldName: "resource." + attr, op: fc.op, value: fc.value} | ||
| spanField := quoteFieldNameIfNeeded(otelpb.SpanAttrPrefixField + attr) | ||
| return "(" + spanField + ":* and " + spanFilter.String() + " or " + spanField + `:"" and ` + resourceFilter.String() + ")" |
There was a problem hiding this comment.
P3: For nil-value ops the generated expression contains a dead sub-filter: {.foo != nil} emits ("span_attr:foo":* and "span_attr:foo":* or ...) (the presence guard is duplicated) and {.foo = nil} emits ("span_attr:foo":* and "span_attr:foo":"" or ...), where the first and branch is never satisfiable and the second branch already requires "span_attr:foo":"". Emit the span filter without re-adding the guard when the filter already encodes presence/absence (nil ops), e.g. "span_attr:foo":"" and "resource_attr:foo":"" for = nil.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At lib/traceql/filter_general.go, line 120:
<comment>For nil-value ops the generated expression contains a dead sub-filter: `{.foo != nil}` emits `("span_attr:foo":* and "span_attr:foo":* or ...)` (the presence guard is duplicated) and `{.foo = nil}` emits `("span_attr:foo":* and "span_attr:foo":"" or ...)`, where the first `and` branch is never satisfiable and the second branch already requires `"span_attr:foo":""`. Emit the span filter without re-adding the guard when the filter already encodes presence/absence (nil ops), e.g. `"span_attr:foo":"" and "resource_attr:foo":""` for `= nil`.</comment>
<file context>
@@ -111,6 +111,15 @@ func (fc *filterCommon) String() string {
+ spanFilter := filterCommon{fieldName: "span." + attr, op: fc.op, value: fc.value}
+ resourceFilter := filterCommon{fieldName: "resource." + attr, op: fc.op, value: fc.value}
+ spanField := quoteFieldNameIfNeeded(otelpb.SpanAttrPrefixField + attr)
+ return "(" + spanField + ":* and " + spanFilter.String() + " or " + spanField + `:"" and ` + resourceFilter.String() + ")"
+ }
+
</file context>
|
|
||
| // Unscoped attribute `.foo` reads the span attribute if the span has it, and the resource attribute otherwise, | ||
| // `.service.name` is mapped to the resource stream field below. | ||
| if attr, ok := strings.CutPrefix(fc.fieldName, "."); ok && fc.fieldName != ".service.name" { |
There was a problem hiding this comment.
P3: Dotted intrinsic names (.name, .kind, .status, .duration) now fall into this unscoped-attribute branch and are resolved as span_attr:/resource_attr: attribute lookups, while the same keywords without the dot resolve to the span name stream field (name), status_code, kind, or the duration field. .status = "error" will never match the status intrinsic, and a span whose literal attribute is named status would now match where it previously didn't. Extend the special-case beyond .service.name (or remap these dotted forms to the intrinsic fields) so unscoped intrinsics keep intrinsic semantics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At lib/traceql/filter_general.go, line 116:
<comment>Dotted intrinsic names (`.name`, `.kind`, `.status`, `.duration`) now fall into this unscoped-attribute branch and are resolved as `span_attr:`/`resource_attr:` attribute lookups, while the same keywords without the dot resolve to the span name stream field (`name`), `status_code`, `kind`, or the `duration` field. `.status = "error"` will never match the status intrinsic, and a span whose literal attribute is named `status` would now match where it previously didn't. Extend the special-case beyond `.service.name` (or remap these dotted forms to the intrinsic fields) so unscoped intrinsics keep intrinsic semantics.</comment>
<file context>
@@ -111,6 +111,15 @@ func (fc *filterCommon) String() string {
+ // Unscoped attribute `.foo` reads the span attribute if the span has it, and the resource attribute otherwise,
+ // `.service.name` is mapped to the resource stream field below.
+ if attr, ok := strings.CutPrefix(fc.fieldName, "."); ok && fc.fieldName != ".service.name" {
+ spanFilter := filterCommon{fieldName: "span." + attr, op: fc.op, value: fc.value}
+ resourceFilter := filterCommon{fieldName: "resource." + attr, op: fc.op, value: fc.value}
</file context>
There was a problem hiding this comment.
In Tempo, a leading . means an attribute rather than an intrinsic. I checked this against Tempo 3.1.0: {.status = "error"} matches an attribute named status, while bare {status = error} matches the built-in status field, which is the same behavior we have here
Vandit1604
left a comment
There was a problem hiding this comment.
This fixes the case in the issue. I tried it with spans that have istio.cluster_id on the span, on the resource, or on both. Each one came back as expected, with the span value taking priority like in Tempo.
The Kiali query from the issue, with | select(...) at the end, works too. TempoAPISearch in apptest/vtsingle.go on master could turn these cases into a test of the returned spans.
I left one note on line 118 about the instrumentation scope.
| // `.service.name` is mapped to the resource stream field below. | ||
| if attr, ok := strings.CutPrefix(fc.fieldName, "."); ok && fc.fieldName != ".service.name" { | ||
| spanFilter := filterCommon{fieldName: "span." + attr, op: fc.op, value: fc.value} | ||
| resourceFilter := filterCommon{fieldName: "resource." + attr, op: fc.op, value: fc.value} |
There was a problem hiding this comment.
Tempo also looks at the instrumentation scope after the resource (protospan.go). So a span that has istio.cluster_id = "c1" only on its scope is missed by =. It also shows up for = nil and != "c1".
The issue only asks for span and resource, so this is fine as a follow-up. If you want to add it here, a third branch on scope_attr: should work, since {instrumentation.istio.cluster_id = "c1"} already returns that span.
There was a problem hiding this comment.
I was wrong here, sorry. I ran Tempo 3.1.0 with the same spans, and {.istio.cluster_id = "c1"} returns only the span and resource matches. The scope-only span comes back only for {instrumentation.istio.cluster_id = "c1"}, so the PR already matches Tempo and you can ignore this note.
e0db758 to
694b544
Compare
Use the span attribute if the span has it, otherwise the resource attribute
.service.nameis excluded so it keeps the stream filter fast pathFixes #198