-
Notifications
You must be signed in to change notification settings - Fork 850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/module name change #22258
Fix/module name change #22258
Conversation
tanyasethi-msft
commented
Jan 17, 2024
- The purpose of this PR is explained in this or a referenced issue.
- The PR does not update generated files.
- These files are managed by the codegen framework at Azure/autorest.go.
- Tests are included and/or updated for code changes.
- Updates to module CHANGELOG.md are included.
- MIT license headers are included in each file.
# Conflicts: # sdk/storage/azdatalake/file/client_test.go
# Conflicts: # sdk/storage/azdatalake/CHANGELOG.md # sdk/storage/azdatalake/internal/exported/version.go
There should ideally be one commit showing here which should be the last one, right? |
@@ -45,12 +45,12 @@ const ( | |||
const crc64Polynomial uint64 = 0x9A6C9329AC4BC9B5 | |||
|
|||
const ( | |||
AppendBlobClient = "azblob/appendblob.Client" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of [email protected]
, the moduleName
param to azcore.NewClient()
takes the full module name instead of the package.ClientName
. So, all of these constants can go away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the three packages are already using [email protected]
. What do you mean by full module name?
The context here is that earlier we were passing ModuleName
as azblob/service.Client
for example to create the azcore client from service client. In the telemetry policy, this is updated to service.Client
from this code. This resulted in the user agent string as azsdk-go-service.Client/v1.2.0 (go1.19.3; Windows_NT)
.
In this PR we are replacing /
with .
in module name of respective clients which would give the user agent string, azsdk-go-azblob.service/v1.2.0 (go1.19.3; Windows_NT)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fully qualified module name e.g. github.com/Azure/azure-sdk-for-go/sdk/storage/azblob
.
Originally, we wanted package.ClientName
for use by the tracing policy. We've retooled how tracing works, so passing package.ClientName
is no longer necessary.
Regarding the telemetry policy, we don't want/need the client name in the User-Agent
string. See the guidelines for the format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In current implementation, "azsdk-go-service.Client/v1.2.0 (go1.19.3; Windows_NT)" this user-agent string is not able to identify whether request was generated by "blob", "datalake" or "files" client as all three can have service client. Hence we are removing the "/" to include "azblob" or "azdatalake" in the string as well.
For the other part "service.Client" does not make much sense for telemetry in any way. If we are interested in just knowing which module was used then "azblob.service" itself tell the whole story ".client" is not adding any value there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On FE side we are building telemetry to parase the User-Agent string and dump some of the keywords to a dgrep table. Longer the string or more vivid the string, higher resource consumption between FE and XArgus. "azsdk-go-blob" shall be good enough for us to understand what SDK was used. Does Azure-SDK team have any sort of telemetry already built to identify which customers are using what SDK or which clients?
Corrected the chageset in #22273 |