Skip to content

Conversation

@prestonvasquez
Copy link
Member

GODRIVER-3659

Summary

This PR implements event filtering for awaitMinPoolSizeMS in the unified test runner, as specified in the unified test format specification.

A naive implementation would clear specific event arrays after initialization:

client.pooled = nil
client.serverDescriptionChanged = nil
client.serverHeartbeatStartedEvent = nil
// ... clear each SDAM event type

However, if we add a new CMAP or SDAM event type in the future, we must remember to update this clearing block. Forgetting to do so means initialization events leak into test assertions, causing false failures.

The eventSequencer assigns a monotonically increasing sequence number to each CMAP and SDAM event as it's recorded. After pool initialization completes, we capture the current sequence as a cutoff. When verifying test expectations, we filter out any events with sequence numbers at or below the cutoff. This approach is future-proof because new event types automatically participate in filtering as long as they call the appropriate recording method in their event processor.

Background & Motivation

PR #2196 added support for awaitMinPoolSizeMS to the unified test runner, but was merged before the specification PR mongodb/specifications#1834 was finalized. As a result, the initial implementation used a simplified approach that doesn't match the final specification requirements.

Per the spec, when awaitMinPoolSizeMS is specified:

Any CMAP and SDAM event/log listeners configured on the client should ignore any events that occur before the pool is being populated.

Copilot AI review requested due to automatic review settings November 8, 2025 00:08
@prestonvasquez prestonvasquez requested a review from a team as a code owner November 8, 2025 00:08
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements event filtering for awaitMinPoolSizeMS in the unified test runner to comply with the specification requirement that CMAP and SDAM events occurring during connection pool initialization should be ignored. The implementation uses a sequence-based filtering approach where events are assigned monotonically increasing sequence numbers, and a cutoff is set after pool initialization completes.

Key Changes:

  • Replaced boolean awaitMinPoolSize field with awaitMinPoolSizeMS integer field to specify timeout duration
  • Introduced eventSequencer to track event ordering via sequence numbers and filter events below a cutoff threshold
  • Modified event processing functions to record sequence numbers for all CMAP and SDAM events

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
internal/integration/unified/entity.go Updated entityOptions to use awaitMinPoolSizeMS with timeout duration instead of boolean flag
internal/integration/unified/client_entity.go Added eventSequencer type and filtering logic; updated awaitMinimumPoolSize to accept timeout parameter and set cutoff after pool initialization
internal/integration/unified/event_verification.go Modified event verification functions to use filterEventsBySeq for filtering CMAP and SDAM events
internal/integration/unified/client_entity_test.go Added comprehensive unit tests for eventSequencer functionality

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +358 to +367
if c.eventSequencer.cutoff == 0 {
return events
}

var filtered []T
for i, evt := range events {
if seqSlice[i] > c.eventSequencer.cutoff {
filtered = append(filtered, evt)
}
}
Copy link

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential index out of bounds panic if seqSlice length doesn't match events length. Add a length check before the loop or validate that i < len(seqSlice) in the loop condition.

Copilot uses AI. Check for mistakes.
return false
}

return es.seqByEventType[eventType][index] <= es.cutoff
Copy link

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential index out of bounds panic if the index is beyond the slice length. Add bounds checking before accessing es.seqByEventType[eventType][index].

Suggested change
return es.seqByEventType[eventType][index] <= es.cutoff
seqs, ok := es.seqByEventType[eventType]
if !ok || index < 0 || index >= len(seqs) {
return false
}
return seqs[index] <= es.cutoff

Copilot uses AI. Check for mistakes.
@mongodb-drivers-pr-bot
Copy link
Contributor

🧪 Performance Results

Commit SHA: 95e4d63

The following benchmark tests for version 690e89e64dcf7a0007e61054 had statistically significant changes (i.e., |z-score| > 1.96):

Benchmark Measurement % Change Patch Value Stable Region H-Score Z-Score
BenchmarkBSONFlatDocumentDecoding ops_per_second_min -41.4294 1295.6187 Avg: 2212.0622
Med: 2257.7594
Stdev: 308.0519
0.8254 -2.9750
BenchmarkBSONDeepDocumentDecoding ops_per_second_min 20.1159 2403.7017 Avg: 2001.1528
Med: 2006.4528
Stdev: 169.0134
0.7919 2.3818
BenchmarkMultiFindMany total_bytes_allocated -10.1039 683296536.0000 Avg: 760096146.2222
Med: 763743356.0000
Stdev: 36028084.7754
0.7443 -2.1317
BenchmarkMultiFindMany total_mem_allocs -9.8681 421791.0000 Avg: 467970.6111
Med: 469972.5000
Stdev: 22195.9290
0.7368 -2.0805

For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch.

@mongodb-drivers-pr-bot
Copy link
Contributor

API Change Report

No changes found!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant