grafana / tempo

Grafana Tempo is a high volume, minimal dependency distributed tracing backend.
https://grafana.com/oss/tempo/
GNU Affero General Public License v3.0
3.91k stars 510 forks source link

[TraceQL] Support attribute existence filter #2188

Open joe-elliott opened 1 year ago

joe-elliott commented 1 year ago

Is your feature request related to a problem? Please describe. There should be a way to query for the existence of an attribute without asserting a value. Perhaps something like:

{ span.att = nil }
{ exists(span.att) }
{ span.att }

Note that the last one overloads querying a boolean attribute for true or false values. If we want to go that route we should think through it carefully.

lanapouney commented 1 year ago

Hi, I am looking for a way to query that an attribute does not exist for a span, could this be achieved via { !exists(span.att) } or would you propose another method for this?

joe-elliott commented 1 year ago

Unfortunately this does not currently exist. I want it too, but haven't found the time to add it. Do you have a preferred syntax above?

If you know the type of your attribute you can do something like { span.att != ""} or `{ span.att != 0 }'. I know that's not great, but it's the only current work around.

lanapouney commented 1 year ago

For me the second syntax makes the most sense. ( { exists(span.att) } )

Unfortunately that doesn't work in this case, my only idea for a work around would be to set att="" on span creation as { span.att != ""} checks that the attribute does exist, and I want to find all spans where is does not exist at all, and in theory my only other option there is to query for if it exists but is only set to an empty string.

cyrille-leclerc commented 1 year ago

I prefer { span.att = nil } and { span.att = nil } because they reuse the existing TraceQL comparison operators and work well IMO for auto-completion

cedricziel commented 1 year ago

I'm putting my money on { .attr = nil } as well because it's simple and intuitive

adirmatzkin commented 1 year ago

Just making sure I don't miss out on something here... By querying for a value of "nil" for some attribute x (syntax as { span.x = nil }), we would assume x doesn't even exist? How would we differentiate between the 2 cases:

  1. x is not even a field in a span
  2. x is a field in a span, but its value is None/Null/Nil.

If we're about to use "nil", "none", or "null" for different meanings here - let me raise a red flag, because it would be confusing and not intuitive for users...

Unless I'm missing something - I feel like the { exists(span.x) } syntax would be better here due to the need to differentiate between the above 2 cases.

cyrille-leclerc commented 1 year ago

Great verification.

From what I understand, null value is not allowed for hotel attributes and it means to suppress the attribute. I have to read in depth https://github.com/open-telemetry/opentelemetry-specification/issues/797

On Thu, May 18, 2023, 10:44 Adir Matzkin @.***> wrote:

Just making sure I don't miss out on something here... By querying for a value of "nil" for some attribute x (syntax as { span.x = nil }), we would assume x doesn't even exist? How would we differentiate between the 2 cases:

  1. x is not even a field in a span
  2. x is a field in a span, but its value is None/Null/Nil.

If we're about to use "nil", "none", or "null" for different meanings here

  • let me raise a red flag, because it would be confusing and not intuitive for users...

Unless I'm missing something - I feel like the { exists(span.x) } syntax would be better here due to the need to differentiate between the above 2 cases.

— Reply to this email directly, view it on GitHub https://github.com/grafana/tempo/issues/2188#issuecomment-1552735974, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADQHK7QPMUG7C5L5D4DD7LXGXOPZANCNFSM6AAAAAAVVOBDHM . You are receiving this because you are subscribed to this thread.Message ID: @.***>

cyrille-leclerc commented 1 year ago

Confirmed, otel attributes MUST be non null.

See https://github.com/open-telemetry/opentelemetry-specification/tree/main/specification/common

"Attribute values of null are not valid and attempting to set a null value is undefined behavior."

With this clarification, I confirm my preference for the span.x = nil syntax.

On Thu, May 18, 2023, 11:34 Cyrille Le Clerc @.***> wrote:

Great verification.

From what I understand, null value is not allowed for hotel attributes and it means to suppress the attribute. I have to read in depth https://github.com/open-telemetry/opentelemetry-specification/issues/797

On Thu, May 18, 2023, 10:44 Adir Matzkin @.***> wrote:

Just making sure I don't miss out on something here... By querying for a value of "nil" for some attribute x (syntax as { span.x = nil }), we would assume x doesn't even exist? How would we differentiate between the 2 cases:

  1. x is not even a field in a span
  2. x is a field in a span, but its value is None/Null/Nil.

If we're about to use "nil", "none", or "null" for different meanings here - let me raise a red flag, because it would be confusing and not intuitive for users...

Unless I'm missing something - I feel like the { exists(span.x) } syntax would be better here due to the need to differentiate between the above 2 cases.

— Reply to this email directly, view it on GitHub https://github.com/grafana/tempo/issues/2188#issuecomment-1552735974, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADQHK7QPMUG7C5L5D4DD7LXGXOPZANCNFSM6AAAAAAVVOBDHM . You are receiving this because you are subscribed to this thread.Message ID: @.***>

adirmatzkin commented 1 year ago

Nice, so I did miss something 🙃 , thanks @cyrille-leclerc!

If that's the case, and it looks like the first two options Joe mentioned in the original comment are the leading ones, I'd like to help push this feature forward - which means at this point helping out getting to a decision 😃.

Here are (IMO) the main considerations we should focus on and take into account when dealing with this kind of syntax question:

1. Expressiveness: Allow users to express a wide range of queries and operations in different query patterns. 2. Readability, simplicity, and consistency: Keep it easy and simple to learn/use/read-understand/maintain, by leveraging familiar conventions that will make it intuitive. 3. Performance: Although syntax won't directly impact performance, design decisions can affect execution efficiency by enabling/limiting optimizations in query processing or execution.

The more I think about it - I'm getting convinced the { span.att = nil } syntax will be the better choice.

I think that unless the performance parameter is a win for the exists(span.attr) - there is a winning option and we can continue moving! 💪🏼 Looking forward to hearing an opinion from someone whose familiar with the TraceQL engine 🙃

joe-elliott commented 1 year ago

Agree with { span.attr = nil } and { span.attr != nil }.

!= nil is relatively simple to implement and could probably be done in a couple days by a dev experienced in the TraceQL parser and engine.

= nil will be quite difficult. The fetch layer all the way down to Parquet is entirely built around finding row numbers that match conditions. To make this work we'd need to pull a list of all row numbers at the span level, and then all row numbers where != nil is true and negate them.

cyrille-leclerc commented 1 year ago

Adding != nil would bring so much value that the inconsistency of lacking of = nil is IMO a minor disadvantage. I support the idea to implement != nil first and postpone the implementation of = nil that we may never prioritize

mdisibio commented 1 year ago

= nil will be quite difficult. The fetch layer all the way down to Parquet is entirely built around finding row numbers that match conditions. To make this work we'd need to pull a list of all row numbers at the span level, and then all row numbers where != nil is true and negate them.

Agree, can't think of a more efficient algorithm and that type of query would always be much slower than != nil. But I do think it's achievable with the current storage layer. Fetch something like status (all the row numbers), the attr with no filtering (opnone), and then let the engine determine the matches.

knylander-grafana commented 1 year ago

The output from the work on this issue can be documented with https://github.com/grafana/tempo/issues/2188

ie-pham commented 1 year ago

https://github.com/grafana/tempo-squad/issues/278

github-actions[bot] commented 12 months ago

This issue has been automatically marked as stale because it has not had any activity in the past 60 days. The next time this stale check runs, the stale label will be removed if there is new activity. The issue will be closed after 15 days if there is no new activity. Please apply keepalive label to exempt this Issue.

zohnannor commented 7 months ago

Is there a way to make a query "if attribute exists, compare to X"? For example, span could have attr attribute or it might not, { name = "span" } finds all spans, { name = "span" && span.attr = "test" } filters out all the spans that don't include such attribute. What I want is a monadic behavior, "find all span attributes, and in case span has attr, it must be "test""

mdisibio commented 7 months ago

"find all span attributes, and in case span has attr, it must be "test""

Something like this: { name = "span" && (span.attr = nil || span.attr = "test") } ? That would work as expected when support for = nil is added.

zohnannor commented 6 months ago

That's unfortunate. The lack of ability to find nil values cannot be solved with a workaround and thus requires me to use two different TraceQL queries.