Do not skip service/operation indexing for firehose spans #2242
Conversation
Signed-off-by: Yuri Shkuro <ys@uber.com>
|
Could you add/update tests to ensure that the service name / operation name to traceID indexes are populated? I think adding indexing to firehose spans makes user experience rather confusing. I feel that perhaps disabling the input fields for tag based search and duration based search when services/operations with firehose spans are selected could lead to better user experience. |
This would require much more extensive changes. We can think about it, but I think the current fix is a compromise - yes, you cannot find those traces by tags, but doing that requires a roundtrip to first find the trace, then confirm that it has a tag, and then fail to find by that tag. Where as the current situation is you get operation name in the dropdown, but no traces are found for it - much easier to run into. |
Codecov Report
@@ Coverage Diff @@
## master #2242 +/- ##
=======================================
Coverage 96.16% 96.16%
=======================================
Files 219 219
Lines 10640 10640
=======================================
Hits 10232 10232
Misses 352 352
Partials 56 56
Continue to review full report at Codecov.
|
| durationNoOperationQuery := &mocks.Query{} | ||
| durationNoOperationQuery.On("Bind", matchEverything()).Return(durationNoOperationQuery) | ||
| durationNoOperationQuery.On("Exec").Return(testCase.durationNoOperationQueryError) | ||
| durationNoOperationQuery.On("String").Return("select from duration_index") | ||
|
|
||
| // Define expected order of queries |
There was a problem hiding this comment.
This comment is rather misleading because one might assume that the expected ordering is enforced on asserts. I think we should remove it.
AssertExpectations asserts that everything specified with On and Return was in fact called as expected. Calls may have occurred in any order.
I'm not sure whether this is possible with testify/mock, but gomock provides InOrder which does enforce ordering of expectations.
|
I agree that it is a compromise and that we should get this in right now. I think it's confusing to not communicate these compromise to UI users who don't get any indication that only certain parts of search work for services setting the firehose tag. In fact, even if they end up on an initial firehose trace (via service/operation search), there is no indication that they are viewing a firehose trace and that there are implications wrt search. |
Signed-off-by: Yuri Shkuro <ys@uber.com>
|
I booked a ticket for UI change: jaegertracing/jaeger-ui#575 |
PR #2090 allowed recording of service and operations in the services/operations tables, but did not index traces by service & operation. This actually made UI even more confusing since the Operations dropdown shows a lot more operations, yet no traces can be found for them.
This change re-enables indexing of firehose traces by service and operation for real. It's a compromise, which brings 3x write amplification for each span (writes from PR #2090 are cached so don't happen often). However, it significantly improves usability and is still much cheaper than indexing all of the span tags.