Do not skip service/operation indexing for firehose spans + a couple fixes #2090
Conversation
|
|
||
| func (s *SpanWriter) writeOtherIndexes(span *model.Span, ds *dbmodel.Span) error { |
There was a problem hiding this comment.
Could you use a more descriptive name here?
There was a problem hiding this comment.
Perhaps this could just be writeIndexes like earlier, but it calls writeServiceOperationIndex? Might simplify the branching in writer.go as well.
| assert.Equal(t, "planet-express", serviceWritten.Load()) | ||
| assert.Equal(t, dbmodel.Operation{ | ||
| ServiceName: "planet-express", | ||
| SpanKind: "", | ||
| OperationName: "package-delivery", | ||
| }, operationWritten.Load()) |
There was a problem hiding this comment.
Are these not tested elsewhere? For e.g., if plugin/storage/cassandra/spanstore/writer.go L183 were removed - wouldn't other tests fail?
There was a problem hiding this comment.
I am not testing whether index writing works, but whether it's invoked for firehose span. This is the only test that makes distinction between two types of indices.
Codecov Report
@@ Coverage Diff @@
## master #2090 +/- ##
=========================================
+ Coverage 96.29% 96.3% +<.01%
=========================================
Files 214 214
Lines 10540 10542 +2
=========================================
+ Hits 10150 10152 +2
+ Misses 331 330 -1
- Partials 59 60 +1
Continue to review full report at Codecov.
|
Signed-off-by: Yuri Shkuro <ys@uber.com>
yurishkuro
force-pushed
the
dont-skip-indexing-ops
branch
from
49f08d1 to
19369d0
Compare
Signed-off-by: Yuri Shkuro <ys@uber.com>
…ymore. which cqlsh prints /opt/cassandra/bin/cqlsh Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
| break | ||
| } | ||
| time.Sleep(500 * time.Microsecond) | ||
| } | ||
| tr.mb.AssertGaugeMetrics(t, | ||
| metricstest.ExpectedMetric{Name: "client_stats.connected_clients", Value: test.expGauge}) | ||
| assert.EqualValues(t, test.expGauge, gaugeValue) |
There was a problem hiding this comment.
Unrelated change to fix a flaky test. There was a race condition with gauge value being 1 inside the loop and then dropping back to 0 by the time the AssertGaugeMetrics was called.
| @@ -3,7 +3,7 @@ | |||
| # This script is used in the Docker image jaegertracing/jaeger-cassandra-schema | |||
| # that allows installing Jaeger keyspace and schema without installing cqlsh. | |||
|
|
|||
| CQLSH=${CQLSH:-"/usr/bin/cqlsh"} | |||
| CQLSH=${CQLSH:-"/opt/cassandra/bin/cqlsh"} | |||
There was a problem hiding this comment.
unrelated change to fix regression in Cassandra tests that were breaking CI.
yurishkuro
changed the title
Which problem is this PR solving?
The transitive dep graphs view uses service & operation index to populate the dropdown. However, when traces are stored in the firehose mode, they are excluded from indexing (to reduce the load on storage), including the service/operation tables, which are actually very inexpensive to write because writes only happen once per 12hr period due to a cache in front of them.
Short description of the changes
Allow service/operation tables to be written even in firehose mode.