[Cosmos .NET SDK] - Adds User Agent to Identify Usage#3876
Conversation
ghost
added
the
area-integrations
|
LGTM. @eerhardt |
| configureSettings?.Invoke(settings); | ||
|
|
||
| var clientOptions = new CosmosClientOptions(); | ||
| clientOptions.ApplicationName = CosmosConstants.CosmosApplicationName; |
There was a problem hiding this comment.
Do we want to set the same for the the EntityFramework component: https://github.com/dotnet/aspire/tree/main/src/Components/Aspire.Microsoft.EntityFrameworkCore.Cosmos?
There was a problem hiding this comment.
I am not sure if we really need to set this. When I look at the definition of CosmosDbContextOptionsBuilder, I do not see an option to set the application name in the entity framework library. Cc: @kirankumarkolli .
There was a problem hiding this comment.
Am also not seeing direct API for it.
@ajcvickers thoughts?
| var cosmosApplicationName = CosmosConstants.CosmosApplicationName; | ||
| if (!string.IsNullOrEmpty(clientOptions.ApplicationName)) | ||
| { | ||
| cosmosApplicationName += clientOptions.ApplicationName; |
There was a problem hiding this comment.
Added the | separator at the moment.
There was a problem hiding this comment.
Is the | the correct separator?
https://azure.github.io/azure-sdk/general_azurecore.html#telemetry-policy
Seems to suggest using a slash might be preferred?
| /// Defines the application name used to interact with the aszure cosmos db. This will be suffixed to the | ||
| /// cosmos user-agent to include with every Azure Cosmos DB service interaction. | ||
| /// </summary> | ||
| internal const string CosmosApplicationName = "AspireCosmosDBClient"; |
There was a problem hiding this comment.
Is there an upper limit on the User Agent? If so, we might want to consider shortening this if possible. Maybe just Aspire as the User Agent already contains the fact that it's a Cosmos DB Client.
There was a problem hiding this comment.
We already have a check in place for validating and trimming the Suffix within the Microsoft.Azure.Documents.UserAgentContainer:
public string Suffix
{
get
{
return suffix;
}
set
{
suffix = value;
if (suffix.Length > 64)
{
suffix = suffix.Substring(0, 64);
}
userAgent = BaseUserAgent + suffix;
userAgentUTF8 = Encoding.UTF8.GetBytes(userAgent);
}
}
So, in case it's larger than the defined size, we will fit it within the 64 character window, which would still contain the required application name prefix for us to track.
There was a problem hiding this comment.
That's exactly the point. Because there is a limit, any character we used for the fixed part takes away from the potential user app name.
There was a problem hiding this comment.
I would agree with @ealsur. Just by using Aspire doesn't make much difference, in terms of identification. With this, we would make extra room for the user provided app name, if any.
Addressed with a new commit.
eerhardt
deleted the
users/kundadebdatta/3789_aspire_introduce_user_agent
branch
|
/backport to release/8.0 |
|
Started backporting to release/8.0: https://github.com/dotnet/aspire/actions/runs/8807930224 |
5 participants
Pull Request Description:
The purpose of this PR is to populate the
ApplicationNameparameter in theCosmosClientOptionsso that the user agent can be generated with the necessary suffix. This will help the cosmos db team to identify if the request was originated from Aspire. A sample of the user agent will look like the below:cosmos-netstandard-sdk/3.39.1|0|X64|Microsoft Windows 10.0.22635|.NET 6.0.29|N|F 00000111|AspireMicrosoftAzureCosmosDBClient|Microsoft Reviewers: Open in CodeFlow