Add application identity to USERAGENT payload (V2) - #4632
Add application identity to USERAGENT payload (V2)#4632cheenamalhotra wants to merge 19 commits into
Conversation
Adds an optional agent identifier to the USERAGENT login feature extension so known middleware (EF Core, SSMS, DacFx, ...) can be told apart from direct SqlClient use. - New public `SqlClientAgent` enum and `SqlConnection.RegisterSqlClientAgent(SqlClientAgent)`. - Registration is process-wide and allowed once, so an application cannot overwrite or spoof an agent set by a library. - Can also be set from App.config via a `SqlClientAgent` section. - Payload format bumped to version 2; the agent id is appended as an optional 8th part only when registered. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: efbed43f-1014-45d7-a9f6-9f04711c281e
There was a problem hiding this comment.
🟡 Changes recommended
The LOGIN7 length race, enum validation, test isolation, and documentation issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds process-wide middleware identification to the USERAGENT login payload.
Changes:
- Adds
SqlClientAgentregistration and configuration APIs. - Extends USERAGENT v2 with an optional agent ID.
- Adds tests, documentation, and samples.
File summaries
| File | Description |
|---|---|
src/Microsoft.Data.SqlClient/tests/UnitTests/UserAgentTests.cs |
Tests payload versioning and agent encoding. |
src/Microsoft.Data.SqlClient/tests/UnitTests/SqlClientAgentTests.cs |
Tests identifiers and configuration parsing. |
src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs |
Verifies LOGIN7 agent transmission. |
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlClientAgentConfigurationTests.cs |
Tests configuration precedence. |
src/Microsoft.Data.SqlClient/tests/FunctionalTests/app.config |
Registers the test agent. |
src/Microsoft.Data.SqlClient/src/Resources/Strings.resx |
Adds registration error messages. |
src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs |
Exposes generated resource accessors. |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UserAgent.cs |
Builds and caches agent payloads. |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs |
Writes agent payloads into LOGIN7. |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs |
Creates agent-related exceptions. |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs |
Adds the registration API. |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlClientAgent.cs |
Defines agents and registration logic. |
src/Microsoft.Data.SqlClient/ref/Microsoft.Data.SqlClient.cs |
Updates the public API contract. |
doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml |
Documents registration behavior. |
doc/samples/SqlConnection_RegisterSqlClientAgent.cs |
Demonstrates middleware registration. |
Review details
Files not reviewed (1)
- src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file
Suppressed comments (1)
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlClientAgentConfigurationTests.cs:20
- The test intent is written as ordinary comments, but test methods require XML
<summary>documentation. Convert this explanation to an XML summary so the new test follows the test documentation contract.
[ConditionalFact(typeof(TestUtility), nameof(TestUtility.IsNetFramework))]
public void AppConfigAgent_PreventsProgrammaticRegistration()
- Files reviewed: 14/15 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Capture the USERAGENT payload once in SendPreLoginHandshake and pass it to WriteLoginData, so a concurrent registration cannot make the reserved feature length disagree with the bytes written. - Restrict RegisterSqlClientAgent to declared enum members. Undeclared numeric ids remain valid in config, where forward compatibility matters. - Serialize ConnectionTests via SimulatedServerTestCollection; it now mutates process-wide registration. - Add XML summary to SqlClientAgentConfigurationTests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: efbed43f-1014-45d7-a9f6-9f04711c281e
There was a problem hiding this comment.
🔵 Needs a closer look
Malformed App.config handling needs isolated regression coverage before approval.
Review details
Files not reviewed (1)
- src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlClientAgent.cs:198
- The new tests validate
Parsedirectly, but none exercises this catch throughLoadFromAppConfig. Consequently, the stated guarantee that an invalid or malformed application configuration cannot turn first use into aTypeInitializationExceptionhas no regression coverage. Add an isolated-process/AppDomain test with a badSqlClientAgentsection that triggers registration loading and verifies the failure is consumed.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UserAgent.cs:39
Valueis always built withagentId: null, so it never has the optional eighth part; onlyGetUcs2Bytescan return that transmitted form. Describing theValueproperty itself with the optional format makes its contract inconsistent with the implementation and the seven-part tests. Clarify that this is the base value and that the encoded login payload may append the agent ID.
/// The format is pipe ('|') delimited into 7 parts, plus an optional
/// 8th part:
///
/// <code>2|MS-MDS|{Driver Version}|{Arch}|{OS Type}|{OS Info}|{Runtime Info}[|{Agent Id}]</code>
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlClientAgentConfigurationTests.cs:24
- Convert the preceding ordinary comment into an XML
<summary>for this test method. The repository's test documentation rules require behavior-focused XML summaries on every test method.
[ConditionalFact(typeof(TestUtility), nameof(TestUtility.IsNetFramework))]
public void AppConfigAgent_PreventsProgrammaticRegistration()
- Files reviewed: 14/15 changed files
- Comments generated: 0 new
- Review effort level: Balanced
- Change SqlClientAgent to int-backed so it needs no CLSCompliant attribute, which the notsupported assembly rejects (CS3021). Identifiers are still bounded to a positive 16-bit range. - Extract LoadAgent so the configuration failure paths are testable, and cover malformed config, invalid id, wrong section type, missing section, and a throwing loader. - Clarify that UserAgent.Value never carries the agent id; only the login payload does. - Convert the App.config test comment to an XML summary. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: efbed43f-1014-45d7-a9f6-9f04711c281e
There was a problem hiding this comment.
🟡 Changes recommended
The public enum’s underlying type and CLS annotations do not match the advertised 16-bit API contract.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file
- Files reviewed: 14/15 changed files
- Comments generated: 1
- Review effort level: Balanced
- Remove the now-unnecessary CLSCompliant attribute from the implementation method. - Document the 16-bit identifier contract on the enum, since it is no longer implied by the underlying type. - Assert the underlying type is Int32 so the CLS-compliant surface cannot regress. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: efbed43f-1014-45d7-a9f6-9f04711c281e
There was a problem hiding this comment.
🔵 Needs a closer look
Public API, configuration, and wire-payload changes require final human review, and two documentation nits remain.
Review details
Files not reviewed (1)
- src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file
Suppressed comments (2)
src/Microsoft.Data.SqlClient/ref/Microsoft.Data.SqlClient.cs:603
- The PR's API example still declares
SqlClientAgent : ushort, but this public surface (andUnderlyingType_IsInt32) intentionally publishes anInt32-backed enum. Please update the PR description to omit: ushortor use: int, so consumers are not given an API signature that differs from the assembly.
public enum SqlClientAgent
src/Microsoft.Data.SqlClient/tests/UnitTests/UserAgentTests.cs:221
- Document the
bytesparameter and return value for this new test helper. The repository's test documentation rules require XML<param>and<returns>elements for helper methods where applicable.
/// <summary>
/// Decode a UCS-2 encoded payload back to its string form.
/// </summary>
private static string Decode(ReadOnlyMemory<byte> bytes) =>
- Files reviewed: 14/15 changed files
- Comments generated: 0 new
- Review effort level: Balanced
doc/samples builds against the published Microsoft.Data.SqlClient package, so it cannot reference an API that has not shipped yet. Move the example into the XML docs alongside the existing App.config example and drop the compiled sample file. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: efbed43f-1014-45d7-a9f6-9f04711c281e
There was a problem hiding this comment.
🔵 Needs a closer look
The PR description incorrectly documents SqlClientAgent as having a ushort underlying type.
Review details
Files not reviewed (1)
- src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file
Suppressed comments (1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlClientAgent.cs:23
- The PR's API example still advertises
public enum SqlClientAgent : ushort, while this declaration, the reference surface, and the new underlying-type test intentionally publishInt32. Because the enum's underlying type is observable, update the PR description to showpublic enum SqlClientAgent(or: int) and describe 16 bits as the validated identifier range rather than the underlying type.
public enum SqlClientAgent
- Files reviewed: 13/14 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
CI failure analysis — none of the 6 failing legs are caused by this change.
The test runs a 6-way cross join over
Why this change can't be the cause: it only appends an optional 8th part to the LOGIN7 USERAGENT payload. No manual test references Re-running to clear the flakes. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The asynchronous open path lacks required coverage, and a new test helper has incomplete XML documentation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.Data.SqlClient/tests/UnitTests/UserAgentTests.cs:268
- Add
<param>and<returns>documentation for this new test helper. The repository's test documentation rules require those elements for helpers with parameters and return values.
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Balanced
|
|
||
| /// <include file='../../../doc/snippets/Microsoft.Data.SqlClient/SqlClientApp.xml' path='docs/members[@name="SqlClientApp"]/SqlClientApp/*' /> | ||
| public enum SqlClientApp | ||
| [System.CLSCompliantAttribute(false)] |
There was a problem hiding this comment.
Since this type is opt-in telemetry for existing apps, I don't think the lack of CLS compliance is a problem. Any existing apps relying on CLS compliance will need to take explicit steps to use this enum and the new SqlConnection.SqlClientApp property - I'm fine with that.
There was a problem hiding this comment.
Agreed. The enum and property remain explicitly non-CLS-compliant.
| [InlineData((ushort)0, (ulong)0, "2|A|B|X64|C|D|E|0|0")] | ||
| [InlineData((ushort)7, (ulong)1, "2|A|B|X64|C|D|E|7|1")] | ||
| [InlineData((ushort)0x00AB, (ulong)0, "2|A|B|X64|C|D|E|AB|0")] | ||
| [InlineData(ushort.MaxValue, (ulong)ushort.MaxValue, "2|A|B|X64|C|D|E|FFFF|FFFF")] |
There was a problem hiding this comment.
Updated the theory to use ulong.MaxValue directly in f087c29.
Expose RegisteredApplication, use field-backed storage, cover sync and async login paths, and exercise the full 64-bit driver property range. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: efbed43f-1014-45d7-a9f6-9f04711c281e
There was a problem hiding this comment.
🟡 Changes recommended
The documented pooled and background-creation telemetry behavior lacks automated coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: efbed43f-1014-45d7-a9f6-9f04711c281e
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: efbed43f-1014-45d7-a9f6-9f04711c281e
There was a problem hiding this comment.
🟡 Changes recommended
The documented connecting-state and pooled-attribution behaviors still lack regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs:779
- This null-owner fallback and the deliberate omission from the pool key define the documented pooled-attribution behavior, but no added test exercises either pooled reuse across different application IDs or background
Min Pool Sizecreation. Please cover those paths with a simulated-server/pool test so a future factory or pool change cannot silently attribute a physical connection to the wrong application.
metrics: Metrics,
registeredApplication: sqlOwningConnection?.RegisteredApplication ?? RegisteredApplication.Unknown);
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: efbed43f-1014-45d7-a9f6-9f04711c281e
There was a problem hiding this comment.
🟡 Changes recommended
The property setter can race with Open, causing the reported property value to differ from the identity sent during login.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: efbed43f-1014-45d7-a9f6-9f04711c281e
mdaigle
left a comment
There was a problem hiding this comment.
Type naming and payload structure look good to me.
Fixes #3201
Implements USERAGENT payload version 2 with controlled application identifiers and driver-owned feature flags.
Summary
API
0x0001-0x7FFF0x8000-0xBFFF0xC000-0xFFFFSet
RegisteredApplicationbeforeOpenorOpenAsync. Assigning it while the connection is opening or open throwsInvalidOperationException. Unregistered values can use the appropriate range through an enum cast.Payload
Example:
With no application set, Application Id is
0.Behavior
Min Pool Sizeconnections reportUnknown.RegisteredApplication.SqlClientAgentandRegisterSqlClientAgentdesign.Tests
Checklist