Fix populating module name in telemetry policy (#20967) (#20971)

SDKs that contain one or more clients in sub-packages could have their
module name incorrectly set in the telemetry string.

Author: jhendrixMSFT

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)
 

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
 }
 

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
 }
 

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
 }

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)
 }