test(instrumentation-express): update to use correct span kind enum; also fix test flakiness (#2421)

This is a follow-up on #2408 (comment)
to update one more usage of SpanKind.* to testUtils.OtlpSpanKind.*.

While testing this locally I also noticed that the instr-express tests
using runTestFixture are flaky. The issue is that TestCollector#sortedSpans
can get the span ordering wrong when spans start in the same millisecond
(the resolution of span.startTimeUnixNano). This happens with Express
middleware spans on a fast machine. I've updated sortedSpans to
fallback to using span.parentSpanId to attempt to get more reliable
sorting.

Author: trentm

packages/opentelemetry-test-utils/src/test-fixtures.ts

@@ -110,14 +110,38 @@ export class TestCollector {
     return this._http.close();
   }
 
-  // Return the spans sorted by start time for testing convenience.
+  /**
+   * Return the spans sorted by which started first, for testing convenience.
+   *
+   * Note: This sorting is a *best effort*. `span.startTimeUnixNano` has
+   * millisecond accuracy, so if multiple spans start in the same millisecond
+   * then this cannot know the start ordering. If `startTimeUnixNano` are the
+   * same, this attempts to get the correct ordering using `parentSpanId` -- a
+   * parent span starts before any of its direct children. This isn't perfect.
+   */
   get sortedSpans(): Array<TestSpan> {
     return this.spans.slice().sort((a, b) => {
       assert(typeof a.startTimeUnixNano === 'string');
       assert(typeof b.startTimeUnixNano === 'string');
       const aStartInt = BigInt(a.startTimeUnixNano);
       const bStartInt = BigInt(b.startTimeUnixNano);
-      return aStartInt < bStartInt ? -1 : aStartInt > bStartInt ? 1 : 0;
+      if (aStartInt < bStartInt) {
+        return -1;
+      } else if (aStartInt > bStartInt) {
+        return 1;
+      } else {
+        // Same startTimeUnixNano, which has millisecond accuracy. This is
+        // common for Express middleware spans on a fast enough dev machine.
+        // Attempt to use spanId/parentSpanId to decide on span ordering.
+        if (a.traceId === b.traceId) {
+          if (a.spanId === b.parentSpanId) {
+            return -1;
+          } else if (a.parentSpanId === b.spanId) {
+            return 1;
+          }
+        }
+        return 0;
+      }
     });
   }
 

plugins/node/opentelemetry-instrumentation-express/test/express.test.ts

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { SpanStatusCode, context, SpanKind, trace } from '@opentelemetry/api';
+import { SpanStatusCode, context, trace } from '@opentelemetry/api';
 import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node';
 import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks';
 import {
@@ -675,7 +675,7 @@ describe('ExpressInstrumentation', () => {
           spans[5].name,
           'request handler - /\\/test\\/regex/'
         );
-        assert.strictEqual(spans[5].kind, SpanKind.SERVER);
+        assert.strictEqual(spans[5].kind, testUtils.OtlpSpanKind.INTERNAL);
         assert.strictEqual(spans[5].parentSpanId, spans[1].spanId);
       },
     });