Closed chris-ramon closed 8 years ago
tl:dr —There's not performance impact introduced to the InfluxDBStore.Trace(...) & InfluxDBStre.Traces(...)
with this PR.
I went ahead and re-run benchmarks from #114 for InfluxDBStore.Trace(...)
& InfluxDBStore.Traces(...)
in order to confirm if there are big performance impact with changes introduced in this PR because were are using unmarshalling/marshalling via: UnmarshalEvents
& MarshalEvent
.
InfluxDBStore.Trace(...)
Before:
$ go test -run=XYZ -bench BenchmarkInfluxDBStoreTrace. -benchtime=40s > trace
$ more trace
PASS
BenchmarkInfluxDBStoreTrace100-4 200 411849069 ns/op
BenchmarkInfluxDBStoreTrace250-4 50 1051788921 ns/op
BenchmarkInfluxDBStoreTrace1000-4 10 4227713513 ns/op
ok sourcegraph.com/sourcegraph/appdash 380.156s
After:
$ go test -run=XYZ -bench BenchmarkInfluxDBStoreTrace[0-9]+ -benchtime=40s > trace
$ more trace
PASS
BenchmarkInfluxDBStoreTrace100-4 100 412541990 ns/op
BenchmarkInfluxDBStoreTrace250-4 50 1079761891 ns/op
BenchmarkInfluxDBStoreTrace1000-4 10 4208731896 ns/op
ok sourcegraph.com/sourcegraph/appdash 294.010s
InfluxDBStore.Traces(...)
Before:
$ go test -run=XYZ -bench BenchmarkInfluxDBStoreTracesDefaultPerPage > traces
$ more traces
PASS
BenchmarkInfluxDBStoreTracesDefaultPerPage-4 100 13433622 ns/op
ok sourcegraph.com/sourcegraph/appdash 10.595s
After:
$ go test -run=XYZ -bench BenchmarkInfluxDBStoreTracesDefaultPerPage > traces
$ more traces
PASS
BenchmarkInfluxDBStoreTracesDefaultPerPage-4 100 13300171 ns/op
ok sourcegraph.com/sourcegraph/appdash 11.540s
I hope to test this change with our app soon to confirm, but this looks like a very good change. This seems much more correct than #119 and using using MarshalEvent/UnmarshalEvent seems correct to me (but also highlights why Appdash should eventually move to a more simplified storage modal).
I'll follow-up with more issues if I spot them in my testing. Thanks Chris! :+1:
Details
Issue: #117
func annotationsFromRow(...)
to include common logic that we use to reassemble data back from InfluxDB fields to spanannotations
. Now it usesfunc annotationsFromEvents(..)
which will discard those annotations that should no be included because were not explicitly save byCollect(...)
but that are present becauseInfluxDB
will return all the fields present onspans
measurement:Client.Request.Method
might contain values for sub-spans but not for root-spans. Since span annotations are non-deterministic, current strategy for query those annotations(fields) is to doselect * from spans ....
, therefore:Client.Request.Method
will be included in the query result even for root-spans: