From 0660a7f0ebfc3067112861829d34f9ec3065c1e9 Mon Sep 17 00:00:00 2001 From: Joel Hendrix Date: Tue, 6 Jun 2023 11:25:33 -0700 Subject: [PATCH] Fix populating module name in telemetry policy (#20967) SDKs that contain one or more clients in sub-packages could have their module name incorrectly set in the telemetry string. --- sdk/azcore/CHANGELOG.md | 1 + sdk/azcore/arm/client.go | 9 ++-- sdk/azcore/core.go | 9 ++-- sdk/azcore/internal/shared/shared.go | 29 +++++++++---- sdk/azcore/internal/shared/shared_test.go | 52 ++++++++++++++++++----- 5 files changed, 72 insertions(+), 28 deletions(-) diff --git a/sdk/azcore/CHANGELOG.md b/sdk/azcore/CHANGELOG.md index 91d9a743ff83..c4da5d78f1da 100644 --- a/sdk/azcore/CHANGELOG.md +++ b/sdk/azcore/CHANGELOG.md @@ -5,6 +5,7 @@ ### Bugs Fixed * Retry policy always clones the underlying `*http.Request` before invoking the next policy. * Added some non-standard error codes to the list of error codes for unregistered resource providers. +* Fixed an issue in `azcore.NewClient()` and `arm.NewClient()` that could cause an incorrect module name to be used in telemetry. ## 1.6.0 (2023-05-04) diff --git a/sdk/azcore/arm/client.go b/sdk/azcore/arm/client.go index 94d018d43537..aa34575f66fb 100644 --- a/sdk/azcore/arm/client.go +++ b/sdk/azcore/arm/client.go @@ -28,12 +28,13 @@ type Client struct { // NewClient creates a new Client instance with the provided values. // This client is intended to be used with Azure Resource Manager endpoints. -// - clientName - the fully qualified name of the client ("package.Client"); this is used by the tracing provider when creating spans +// - clientName - the fully qualified name of the client ("module/package.Client"); this is used by the telemetry policy and tracing provider. +// if module and package are the same value, the "module/" prefix can be omitted. // - moduleVersion - the version of the containing module; used by the telemetry policy // - cred - the TokenCredential used to authenticate the request // - options - optional client configurations; pass nil to accept the default values func NewClient(clientName, moduleVersion string, cred azcore.TokenCredential, options *ClientOptions) (*Client, error) { - pkg, err := shared.ExtractPackageName(clientName) + mod, client, err := shared.ExtractModuleName(clientName) if err != nil { return nil, err } @@ -52,12 +53,12 @@ func NewClient(clientName, moduleVersion string, cred azcore.TokenCredential, op if c, ok := options.Cloud.Services[cloud.ResourceManager]; ok { ep = c.Endpoint } - pl, err := armruntime.NewPipeline(pkg, moduleVersion, cred, runtime.PipelineOptions{}, options) + pl, err := armruntime.NewPipeline(mod, moduleVersion, cred, runtime.PipelineOptions{}, options) if err != nil { return nil, err } - tr := options.TracingProvider.NewTracer(clientName, moduleVersion) + tr := options.TracingProvider.NewTracer(client, moduleVersion) return &Client{ep: ep, pl: pl, tr: tr}, nil } diff --git a/sdk/azcore/core.go b/sdk/azcore/core.go index 72c2cf21eef3..27231ad920d5 100644 --- a/sdk/azcore/core.go +++ b/sdk/azcore/core.go @@ -76,12 +76,13 @@ type Client struct { } // NewClient creates a new Client instance with the provided values. -// - clientName - the fully qualified name of the client ("package.Client"); this is used by the tracing provider when creating spans +// - clientName - the fully qualified name of the client ("module/package.Client"); this is used by the telemetry policy and tracing provider. +// if module and package are the same value, the "module/" prefix can be omitted. // - moduleVersion - the semantic version of the containing module; used by the telemetry policy // - plOpts - pipeline configuration options; can be the zero-value // - options - optional client configurations; pass nil to accept the default values func NewClient(clientName, moduleVersion string, plOpts runtime.PipelineOptions, options *ClientOptions) (*Client, error) { - pkg, err := shared.ExtractPackageName(clientName) + mod, client, err := shared.ExtractModuleName(clientName) if err != nil { return nil, err } @@ -96,9 +97,9 @@ func NewClient(clientName, moduleVersion string, plOpts runtime.PipelineOptions, } } - pl := runtime.NewPipeline(pkg, moduleVersion, plOpts, options) + pl := runtime.NewPipeline(mod, moduleVersion, plOpts, options) - tr := options.TracingProvider.NewTracer(clientName, moduleVersion) + tr := options.TracingProvider.NewTracer(client, moduleVersion) return &Client{pl: pl, tr: tr}, nil } diff --git a/sdk/azcore/internal/shared/shared.go b/sdk/azcore/internal/shared/shared.go index 930ab8c83999..db0aaa7cb956 100644 --- a/sdk/azcore/internal/shared/shared.go +++ b/sdk/azcore/internal/shared/shared.go @@ -13,7 +13,6 @@ import ( "reflect" "regexp" "strconv" - "strings" "time" ) @@ -79,14 +78,26 @@ func ValidateModVer(moduleVersion string) error { return nil } -// ExtractPackageName returns "package" from "package.Client". +// ExtractModuleName returns "module", "package.Client" from "module/package.Client" or +// "package", "package.Client" from "package.Client" when there's no "module/" prefix. // If clientName is malformed, an error is returned. -func ExtractPackageName(clientName string) (string, error) { - pkg, client, ok := strings.Cut(clientName, ".") - if !ok { - return "", fmt.Errorf("missing . in clientName %s", clientName) - } else if pkg == "" || client == "" { - return "", fmt.Errorf("malformed clientName %s", clientName) +func ExtractModuleName(clientName string) (string, string, error) { + // uses unnamed capturing for "module", "package.Client", and "package" + regex, err := regexp.Compile(`^(?:([a-z0-9]+)/)?(([a-z0-9]+)\.(?:[A-Za-z0-9]+))$`) + if err != nil { + return "", "", err } - return pkg, nil + + matches := regex.FindStringSubmatch(clientName) + if len(matches) < 4 { + return "", "", fmt.Errorf("malformed clientName %s", clientName) + } + + // the first match is the entire string, the second is "module", the third is + // "package.Client" and the fourth is "package". + // if there was no "module/" prefix, the second match will be the empty string + if matches[1] != "" { + return matches[1], matches[2], nil + } + return matches[3], matches[2], nil } diff --git a/sdk/azcore/internal/shared/shared_test.go b/sdk/azcore/internal/shared/shared_test.go index d868bc6e035d..f283d8921f31 100644 --- a/sdk/azcore/internal/shared/shared_test.go +++ b/sdk/azcore/internal/shared/shared_test.go @@ -85,24 +85,54 @@ func TestValidateModVer(t *testing.T) { require.Error(t, ValidateModVer("v1.2")) } -func TestExtractPackageName(t *testing.T) { - pkg, err := ExtractPackageName("package.Client") +func TestExtractModuleName(t *testing.T) { + mod, client, err := ExtractModuleName("module/package.Client") require.NoError(t, err) - require.Equal(t, "package", pkg) + require.Equal(t, "module", mod) + require.Equal(t, "package.Client", client) - pkg, err = ExtractPackageName("malformed") + mod, client, err = ExtractModuleName("malformed/") require.Error(t, err) - require.Empty(t, pkg) + require.Empty(t, mod) + require.Empty(t, client) - pkg, err = ExtractPackageName(".malformed") + mod, client, err = ExtractModuleName("malformed/malformed") require.Error(t, err) - require.Empty(t, pkg) + require.Empty(t, mod) + require.Empty(t, client) - pkg, err = ExtractPackageName("malformed.") + mod, client, err = ExtractModuleName("malformed/malformed.") require.Error(t, err) - require.Empty(t, pkg) + require.Empty(t, mod) + require.Empty(t, client) - pkg, err = ExtractPackageName("") + mod, client, err = ExtractModuleName("malformed/.malformed") require.Error(t, err) - require.Empty(t, pkg) + require.Empty(t, mod) + require.Empty(t, client) + + mod, client, err = ExtractModuleName("package.Client") + require.NoError(t, err) + require.Equal(t, "package", mod) + require.Equal(t, "package.Client", client) + + mod, client, err = ExtractModuleName("malformed") + require.Error(t, err) + require.Empty(t, mod) + require.Empty(t, client) + + mod, client, err = ExtractModuleName(".malformed") + require.Error(t, err) + require.Empty(t, mod) + require.Empty(t, client) + + mod, client, err = ExtractModuleName("malformed.") + require.Error(t, err) + require.Empty(t, mod) + require.Empty(t, client) + + mod, client, err = ExtractModuleName("") + require.Error(t, err) + require.Empty(t, mod) + require.Empty(t, client) }