Clarify attributes support in API_v3 request query parameters to find traces

[pavolloffay]

Created from jaegertracing/jaeger-idl#76 (comment)

The current definition:

// Query parameters to find traces.
// Note that some storage implementations do not guarantee the correct implementation of all parameters.
message TraceQueryParameters {
  string service_name = 1;
  string operation_name = 2;
  // Attributes are matched against Span and Resource attributes.
  // At least one span in a trace must match all specified attributes.
  map<string, string> attributes = 3;

@jpkrohling suggests thinking about changing the semantics of attributes.

[yurishkuro]

My preference is to clearly document that the filter parameters represent a conjunction on the same span. Expressing conditions that could apply to multiple spans, like endpoint=X on one span and error=true on any other span, requires non-trivial query language, and is not trivial to support e.g. on ES.

[jpkrohling]

For reference, this is the case I had in mind:

Another way to see this: what would you expect as a user? If you specify two pairs of attributes, would you expect them to exist throughout the trace, or for all attributes to exist as part of the same span? I'm not quite sure I have an answer here... Here's one case advocating for attributes to exist throughout the trace:

• root span has "userID=123", no other spans contain this
• span (not root span) has error=true
• SRE is looking for traces for user 123 with errors in it

[jpkrohling]

requires non-trivial query language, and is not trivial to support e.g. on ES

One more reason to choose one backing storage and make the best tracing solution based on it. What I hear you saying is that we are OK to compromise the user experience in favor of supporting multiple backing stores, settling on the minimum common denominator.

[yurishkuro]

we are OK to compromise the user experience in favor of supporting multiple backing stores

I am not saying that, I am saying that the docs / API must reflect the reality of what we actually support. I am also saying that to support the use case you cite we need not only the support of the backing store, but also a different query language, because in the current API your query simply cannot be expressed without internal contradiction (namely, there is a mandatory serviceName field that is meant to match every span otherwise matching the query). If we want to support matching conditions against more than one span, than I would much rather have that explicitly expressed in the query.

[jpkrohling]

I am saying that the docs / API must reflect the reality of what we actually support.

True. Note that this is a new version of the API (v3), we have the opportunity to change the current behavior and the documentation.

I am also saying that to support the use case you cite we need not only the support of the backing store, but also a different query language, because in the current API your query simply cannot be expressed without internal contradiction (namely, there is a mandatory serviceName field that is meant to match every span otherwise matching the query)

Might be just a way of how we present the search form to users. Keeping the requirement to provide the service name, the query could be "give me traces touching the service X, with spans anywhere in the trace containing the userID=123 and spans anywhere containing an attribute error=false."

By query language, you didn't mean a formal language, right?

[yurishkuro]

this is a new version of the API (v3), we have the opportunity to change the current behavior and the documentation.

(a) are you sure changing the behavior is in scope of what Pavol had in mind for this project? (b) I don't mind evolving the API in such a way as to allow it to express the cross-span queries and then degrade to current behavior with certain restrictions.

Might be just a way of how we present the search form to users.

Well, no, we'd need to change the API itself to be expressive. We actually discuss this in the past that the UI representation of the query may need to be more nuanced, i.e. dependent on the capabilities of the backing store, because a generic search form always leads to confusion when not every combination can be supported.

Keeping the requirement to provide the service name

Ironically, ES implementation does not impose this as a requirement, only our indexing method in Cassandra does.

By query language, you didn't mean a formal language, right?

Right, I just mean the API.

If I were to make a concrete proposal, I would say that the way to address your use case is to make TraceQueryParameters a repeatable field, treating repetition as conjunction (AND) across multiple spans of the same trace, while interpreting all fields within that struct as a conjunction on a single span. The only change, if we did it this way, would be to pull out the time start/end out because the time range should really apply once to the whole query.

This would still not address the aspect of duration being applicable to the whole trace vs. individual spans, but I am not actually convinced that this is a very well defined use case, because in practice I often see that the "root" of the trace is a pretty arbitrary node in the graph, especially in systems that allow upsampling. However, it is actually very easy to support by adding a Boolean rootOnly flag to the struct (although I don't think we'd be able to support that in Cassandra).