-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not skip service/operation indexing for firehose spans + a couple fixes #2090
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unrelated change to fix regression in Cassandra tests that were breaking CI. |
||
CQLSH_HOST=${CQLSH_HOST:-"cassandra"} | ||
CQLSH_SSL=${CQLSH_SSL:-""} | ||
CASSANDRA_WAIT_TIMEOUT=${CASSANDRA_WAIT_TIMEOUT:-"60"} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ import ( | |
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/mock" | ||
"github.com/uber/jaeger-lib/metrics/metricstest" | ||
"go.uber.org/atomic" | ||
"go.uber.org/zap" | ||
|
||
"github.com/jaegertracing/jaeger/model" | ||
|
@@ -352,23 +353,36 @@ func TestStorageMode_IndexOnly_WithFilter(t *testing.T) { | |
|
||
func TestStorageMode_IndexOnly_FirehoseSpan(t *testing.T) { | ||
withSpanWriter(0, func(w *spanWriterTest) { | ||
|
||
w.writer.serviceNamesWriter = func(serviceName string) error { return nil } | ||
w.writer.operationNamesWriter = func(operation dbmodel.Operation) error { return nil } | ||
serviceWritten := atomic.NewString("") | ||
operationWritten := &atomic.Value{} | ||
w.writer.serviceNamesWriter = func(serviceName string) error { | ||
serviceWritten.Store(serviceName) | ||
return nil | ||
} | ||
w.writer.operationNamesWriter = func(operation dbmodel.Operation) error { | ||
operationWritten.Store(operation) | ||
return nil | ||
} | ||
span := &model.Span{ | ||
TraceID: model.NewTraceID(0, 1), | ||
TraceID: model.NewTraceID(0, 1), | ||
OperationName: "package-delivery", | ||
Process: &model.Process{ | ||
ServiceName: "service-a", | ||
ServiceName: "planet-express", | ||
}, | ||
Flags: model.Flags(8), | ||
} | ||
|
||
err := w.writer.WriteSpan(span) | ||
assert.NoError(t, err) | ||
w.session.AssertExpectations(t) | ||
w.session.AssertNotCalled(t, "Query", stringMatcher(serviceOperationIndex)) | ||
w.session.AssertNotCalled(t, "Query", stringMatcher(serviceNameIndex)) | ||
w.session.AssertNotCalled(t, "Query", stringMatcher(durationIndex)) | ||
assert.Equal(t, "planet-express", serviceWritten.Load()) | ||
assert.Equal(t, dbmodel.Operation{ | ||
ServiceName: "planet-express", | ||
SpanKind: "", | ||
OperationName: "package-delivery", | ||
}, operationWritten.Load()) | ||
Comment on lines
+380
to
+385
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
}, StoreIndexesOnly()) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
#!/bin/bash | ||
|
||
set -e | ||
set -ex | ||
|
||
# Clean up before starting. | ||
docker rm -f cassandra || true | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.