Skip to content
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(bigtable): Correct the 'method' label value #11350

Merged
merged 2 commits into from
Dec 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bigtable/bigtable.go
Original file line number Diff line number Diff line change
Expand Up @@ -1605,7 +1605,7 @@ func gaxInvokeWithRecorder(ctx context.Context, mt *builtinMetricsTracer, method
f func(ctx context.Context, headerMD, trailerMD *metadata.MD, _ gax.CallSettings) error, opts ...gax.CallOption) error {
attemptHeaderMD := metadata.New(nil)
attempTrailerMD := metadata.New(nil)
mt.method = method
mt.setMethod(method)

var callWrapper func(context.Context, gax.CallSettings) error
if !mt.builtInEnabled {
Expand Down
4 changes: 4 additions & 0 deletions bigtable/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,10 @@ type builtinMetricsTracer struct {
currOp opTracer
}

func (b *builtinMetricsTracer) setMethod(m string) {
b.method = "Bigtable." + m
}

// opTracer is used to record metrics for the entire operation, including retries.
// Operation is a logical unit that represents a single method invocation on client.
// The method might require multiple attempts/rpcs and backoff logic to complete
Expand Down
59 changes: 53 additions & 6 deletions bigtable/metrics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"encoding/base64"
"fmt"
"io"
"slices"
"sort"
"strings"
"sync"
Expand Down Expand Up @@ -166,17 +167,17 @@ func TestNewBuiltinMetricsTracerFactory(t *testing.T) {
}{
{
desc: "should create a new tracer factory with default meter provider",
config: ClientConfig{},
config: ClientConfig{AppProfile: appProfile},
wantBuiltinEnabled: true,
wantCreateTSCallsCount: 2,
},
{
desc: "should create a new tracer factory with noop meter provider",
config: ClientConfig{MetricsProvider: NoopMetricsProvider{}},
config: ClientConfig{MetricsProvider: NoopMetricsProvider{}, AppProfile: appProfile},
},
{
desc: "should not create instruments when BIGTABLE_EMULATOR_HOST is set",
config: ClientConfig{},
config: ClientConfig{AppProfile: appProfile},
setEmulator: true,
},
}
Expand All @@ -201,9 +202,8 @@ func TestNewBuiltinMetricsTracerFactory(t *testing.T) {
t.Errorf("builtinEnabled: got: %v, want: %v", gotClient.metricsTracerFactory.enabled, test.wantBuiltinEnabled)
}

if diff := testutil.Diff(gotClient.metricsTracerFactory.clientAttributes, wantClientAttributes,
cmpopts.IgnoreUnexported(attribute.KeyValue{}, attribute.Value{})); diff != "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this as all the fields in attribute.Value are unexported and nothing was getting compared

https://github.com/open-telemetry/opentelemetry-go/blob/v1.29.0/attribute/value.go#L21-L27

t.Errorf("clientAttributes: got=-, want=+ \n%v", diff)
if !equalsKeyValue(gotClient.metricsTracerFactory.clientAttributes, wantClientAttributes) {
t.Errorf("clientAttributes: got: %+v, want: %+v", gotClient.metricsTracerFactory.clientAttributes, wantClientAttributes)
}

// Check instruments
Expand Down Expand Up @@ -265,7 +265,26 @@ func TestNewBuiltinMetricsTracerFactory(t *testing.T) {
for _, gotCreateTSCall := range gotCreateTSCalls {
gotMetricTypes := []string{}
for _, ts := range gotCreateTSCall.TimeSeries {
// ts.Metric.Type is of the form "bigtable.googleapis.com/internal/client/server_latencies"
gotMetricTypes = append(gotMetricTypes, ts.Metric.Type)

// Assert "streaming" metric label is correct
gotStreaming, gotStreamingExists := ts.Metric.Labels[metricLabelKeyStreamingOperation]
splitMetricType := strings.Split(ts.Metric.Type, "/")
internalMetricName := splitMetricType[len(splitMetricType)-1] // server_latencies
wantStreamingExists := slices.Contains(metricsDetails[internalMetricName].additionalAttrs, metricLabelKeyStreamingOperation)
if wantStreamingExists && (!gotStreamingExists || gotStreaming != "true") {
t.Errorf("Metric label key: %s, value: got: %v, want: %v", metricLabelKeyStreamingOperation, gotStreaming, "true")
}
if !wantStreamingExists && gotStreamingExists {
t.Errorf("Metric label key: %s exists, value: got: %v, want: %v", metricLabelKeyStreamingOperation, gotStreamingExists, wantStreamingExists)
}

// Assert "method" metric label is correct
wantMethod := "Bigtable.ReadRows"
if gotLabel, ok := ts.Metric.Labels[metricLabelKeyMethod]; !ok || gotLabel != wantMethod {
t.Errorf("Metric label key: %s, value: got: %v, want: %v", metricLabelKeyMethod, gotLabel, wantMethod)
}
}
sort.Strings(gotMetricTypes)
if !testutil.Equal(gotMetricTypes, wantMetricTypesGCM) {
Expand All @@ -289,6 +308,34 @@ func setMockErrorHandler(t *testing.T, mockErrorHandler *MockErrorHandler) {
})
}

func equalsKeyValue(gotAttrs, wantAttrs []attribute.KeyValue) bool {
if len(gotAttrs) != len(wantAttrs) {
return false
}

gotJSONVals, err := keyValueToKeyJSONValue(gotAttrs)
if err != nil {
return false
}
wantJSONVals, err := keyValueToKeyJSONValue(wantAttrs)
if err != nil {
return false
}
return testutil.Equal(gotJSONVals, wantJSONVals)
}

func keyValueToKeyJSONValue(attrs []attribute.KeyValue) (map[string]string, error) {
keyJSONVal := map[string]string{}
for _, attr := range attrs {
jsonVal, err := attr.Value.MarshalJSON()
if err != nil {
return nil, err
}
keyJSONVal[string(attr.Key)] = string(jsonVal)
}
return keyJSONVal, nil
}

func TestExporterLogs(t *testing.T) {
ctx := context.Background()
project := "test-project"
Expand Down
Loading