CSHARP-5888: Optimize CommandEventHelper to avoid redundant message decoding#1891
CSHARP-5888: Optimize CommandEventHelper to avoid redundant message decoding#1891adelinowona wants to merge 4 commits intomongodb:mainfrom
Conversation
adelinowona
added
the
improvement
adelinowona
requested review from
BorisDog and
sanych-sun
and removed request for
papafe
There was a problem hiding this comment.
Pull request overview
This pull request optimizes the CommandEventHelper to avoid redundant message decoding when CommandStartedEvent tracking is disabled and query text extraction is not needed. The optimization extracts sessionId, transactionNumber, and databaseNamespace directly from the Type0CommandMessageSection without decoding the entire command message.
Changes:
- Added sessionId, transactionNumber, and databaseNamespace as properties on Type0CommandMessageSection for direct access
- Modified CommandEventHelper to conditionally decode messages only when CommandStartedEvent or query text is needed
- Updated MongoTelemetry.StartCommandActivity to accept pre-extracted sessionId and transactionNumber parameters
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/MongoDB.Driver/MongoTelemetry.cs | Added sessionId and transactionNumber parameters to StartCommandActivity; moved System.Net.Sockets using inside conditional compilation block |
| src/MongoDB.Driver/Core/WireProtocol/Messages/CommandMessageSection.cs | Added optional databaseNamespace, sessionId, and transactionNumber parameters to Type0CommandMessageSection constructors |
| src/MongoDB.Driver/Core/WireProtocol/CommandUsingCommandMessageWireProtocol.cs | Captures sessionId and transactionNumber values and passes them to Type0CommandMessageSection constructor |
| src/MongoDB.Driver/Core/WireProtocol/CommandMessageFieldEncryptor.cs | Preserves databaseNamespace, sessionId, and transactionNumber when creating encrypted message sections |
| src/MongoDB.Driver/Core/Connections/CommandEventHelper.cs | Implements optimization to avoid message decoding when not needed; extracts metadata from Type0CommandMessageSection directly |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| if (command.TryGetValue("txnNumber", out var txnNumber)) | ||
| if (transactionNumber.HasValue) |
There was a problem hiding this comment.
Should we try to get the txnNumber from the command if it's not provided as explicit argument? Same way as we doing for sessionId.
There was a problem hiding this comment.
the only place where it is not provided as an explicit argument is in the OP_QUERY message case which doesn't support transactions so it will never contain any transaction numbers in the command.
| bool moreToCome; | ||
|
|
||
| // Only decode when we need the full command for CommandStartedEvent or db.query.text | ||
| if (_shouldTrackStarted || _queryTextMaxLength > 0) |
There was a problem hiding this comment.
Minor : We also do not need querytext when shouldRedactCommand is true.
| command = shouldRedactCommand ? new BsonDocument() : originalCommand; | ||
| moreToCome = message.WrappedMessage.MoreToCome; | ||
|
|
||
| if (_shouldTrackStarted) |
There was a problem hiding this comment.
Nice, so this actually approves the performance in case of CommandStartedEvent?
How is the command in this case different from the previous flow?
There was a problem hiding this comment.
Not really this is just a small optimization where if the command is going to be redacted then we also avoid doing the decoding but we still need to record the command event if anyone is subscribed to that.
So the condition is essentially to decode only when we need the full command content (started event or query text logging) AND the command isn't redacted. Otherwise, skip decoding but still do whatever bookkeeping is needed.
| stopwatch, | ||
| new CollectionNamespace(databaseNamespace, "$cmd"), | ||
| decodedMessage.MoreToCome ? ExpectedResponseType.None : ExpectedResponseType.Command, | ||
| moreToCome ? ExpectedResponseType.None : ExpectedResponseType.Command, |
There was a problem hiding this comment.
minor, not need for a separate moreToCome variable ?
No description provided.