Update storage api to support query operations by spanKind

[guo0693]

Which problem is this PR solving?

• Relates to Cassandra & memory storage update: query service to support spanKind when retrieve operations for a given service. jaegertracing/jaeger#1921

Short description of the changes

• storage API changes to support new query parameter and return type (across all storage engines)

[yurishkuro]

overall changes look ok-ish, but I still think we should limit the blast raduis and not require filtering logic to be done in the storage, just let it return all ops for a service, and do the filtering in the QueryService only (one place).

[guo0693]

overall changes look ok-ish, but I still think we should limit the blast raduis and not require filtering logic to be done in the storage, just let it return all ops for a service, and do the filtering in the QueryService only (one place).

Just want to make sure your proposal is for avoiding potential duplicate filtering logic, not for reducing the number of affected files (I assume blast radius means the number of affected files). Because as long as we touch the interface (since spanKind has to be returned in the Operation struct), all the implementations need to be changed.

GetOperations(ctx context.Context, string service) ([]*Operation, error)
GetOperations(ctx context.Context, query *OperationQueryParameters) ([]*Operation, error)
The affected files will be the same for the above two versions.
The benefit of the second method is:

1. we can easily add more param into OperationQueryParameters without modifying so many files in the future
2. we avoid storage plugin returning un-necessary kind of operations followed by filtering. Although the performance gain is tiny given the number of operations won't be too many, I would prefer this because filtering in query_service does not reduce the complexity of the changes.

Regarding logic duplication, if the storage support filtering in the query statement, then we are not really duplicate the filtering logic in the code.

[yurishkuro]

My point is that you're increasing the scope by requiring storage to implement filtering, which forces you to change memory storage implementation, for example. If you implement it once in the QueryService then all storage impls benefit automatically, whether they support internal filtering or not (e.g. do you know how to implement query-time filtering in Badger?)

but ok, let's do it your way.

[guo0693]

My point is that you're increasing the scope by requiring storage to implement filtering, which forces you to change memory storage implementation, for example. If you implement it once in the QueryService then all storage impls benefit automatically, whether they support internal filtering or not (e.g. do you know how to implement query-time filtering in Badger?)

but ok, let's do it your way.

Those storage plugins have to be changed anyway in order to write & read the extra spanKind, they might just as well add the filtering when they make the changes since it's pretty easy to them. I will make the changes in proto and break long lines then push another commit.

Btw, I think we might need to keep the following so that old proto users don't need to change their code, otherwise they need to use the corresponding updated get or set method:

message GetOperationsResponse {
    repeated string operation = 1; // deprecated
    repeated Operation operations_v2 = 2;
}

IIRC, the backward compatibility when change names only kicks in when you unmarshall saved data into new structure.

[yurishkuro]

they might just as well add the filtering when they make the changes since it's pretty easy.

you don't know if it's easy or not, it depends on storage implementation (I wouldn't know how to make Badger return only filtered records). And if you're talking about filtering in Go code, then that's exactly the duplication I am referring to.

[guo0693]

Weird, this line of code is within the vertical 120 char line in my local intelliJ, somehow github still shows scroll bar.

[guo0693]

I need to reopen the PR with same changes against master since jaegertracing#1921-2 is merged