Refactors integration tests to be more isolated #3249
Conversation
|
fwiw I don't expect anyone to go over these lines. the main thing here was rejigging tests so that cassandra can re-use a keyspace between methods, as in v4 of the driver there's a 10s overhead to install our schema. This is about the same as starting the whole container! |
This makes all integration tests use unique data. Notably, they no longer use constant trace ID or service names, as this can lead to difficulty when a keyspace is re-used. For example, service names and trace IDs are often partitions. When debugging it is easier when test data is isolated by a service name as it can be easily queried. This also pulls the heaviest tests into their own classes so that they don't overload the storage containers used by the bulk of our tests. Finally, this fixes a few glitches in the v1 cassandra storage.
a36ca5b to
4949831
Compare
|
Flake is the usual ES one (after cassandra passed) |
* 2_22_0: (108 commits) [maven-release-plugin] prepare release 2.22.0 Hardens integration tests, fixes small bug (openzipkin#3258) Deprecates Cassandra v1 schema for removal in Zipkin 2.23 (openzipkin#3254) Update Armeria to 1.2.0 and Netty to 4.1.53 (openzipkin#3257) Removes JMX dependency from our docker configuration (openzipkin#3252) Quiets startup logging (openzipkin#3253) Removes Cassandra querybuilder dependency (openzipkin#3250) Migrates to Datastax Driver v4 (openzipkin#3246) Refactors integration tests to be more isolated (openzipkin#3249) Adds workaround to missed decorator route (openzipkin#3245) disallow trace requests (openzipkin#3239) Refactors Cassandra queries so they are cheaper and easier to migrate (openzipkin#3243) fix docker hook CWD Builds builder with same script as other images (openzipkin#3242) Removes last clutter from zipkin startup (openzipkin#3240) Stops docker-compose from being mistaken as a workflow (openzipkin#3241) fix master Finishes Docker refactoring (openzipkin#3238) Bumps versions and applies failsafe workaround (openzipkin#3235) Fix a bug in the unit of duration (openzipkin#3234) ...
| @@ -635,6 +635,7 @@ public static String normalizeTraceId(String traceId) { | |||
| if (length > 32) throw new IllegalArgumentException("traceId.length > 32"); | |||
| int zeros = validateHexAndReturnZeroPrefix(traceId); | |||
| if (zeros == length) throw new IllegalArgumentException("traceId is all zeros"); | |||
| if (length == 15) throw new RuntimeException("WTF"); | |||
There was a problem hiding this comment.
@adriancole can you provide us more context about this change? We were implicitly relying on the previous logic that left-padded 0s for trace ids generated by a legacy application. We have a workaround in place to fix the broken pipeline, but we wanted to hear why this explicitly checks for length of 15 (vs 14, 13 etc...)
There was a problem hiding this comment.
@codefromthecrypt Why was this change added? Any other hex code (eg of length 14) will be nicely prepended with a leading zero, but why should the 15 throw an exception exactly?
There was a problem hiding this comment.
guess by the exception name that this was an accident especially as there's logic later to handle padding
There was a problem hiding this comment.
I'll create a PR to remove this change then
There was a problem hiding this comment.
#3471 has been created. In case something is off with it, let's continue any discussion there.
Thanks for the quick replies!
This makes all integration tests use unique data. Notably, they no
longer use constant trace ID or service names, as this can lead to
difficulty when a keyspace is re-used. For example, service names and
trace IDs are often partitions. When debugging it is easier when test
data is isolated by a service name as it can be easily queried.
This also pulls the heaviest tests into their own classes so that they
don't overload the storage containers used by the bulk of our tests.
Finally, this fixes a few glitches in the v1 cassandra storage.