-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
chore(spanner): add support of client side native metrics collection and export #10419
Conversation
26dc986
to
faf5cdb
Compare
765633c
to
ee86524
Compare
2d2b5a6
to
23ca77a
Compare
c507ea3
to
7ba036f
Compare
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.
Please separate non-standard library imports from standard library imports.
import ( | ||
"cloud.google.com/go/spanner" | ||
"context" | ||
"github.com/googleapis/gax-go/v2" | ||
"google.golang.org/api/option" | ||
) |
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.
Stylistically in Go, please separate standard library imports from other imports so
import (
"context"
"cloud.google.com/go/spanner"
"github.com/googleapis/gax-go/v2"
"google.golang.org/api/option"
)
spanner/metrics.go
Outdated
"github.com/google/uuid" | ||
"github.com/googleapis/gax-go/v2" | ||
"go.opentelemetry.io/otel/metric/noop" |
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.
Please move these downloads out of the standard library imports block.
spanner/metrics.go
Outdated
package spanner | ||
|
||
import ( | ||
"cloud.google.com/go/spanner/internal" |
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.
This too, please move down.
95db2af
to
1affbec
Compare
@igorbernstein2 @codyoss Note: PR message is chore intentionally since we want this feature disabled(will be done in next commit here) but other things can be reviewed. |
1affbec
to
fae02f9
Compare
spanner/metrics.go
Outdated
|
||
clientName = fmt.Sprintf("go-spanner v%v", internal.Version) | ||
|
||
bucketBounds = []float64{0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 8.0, 10.0, 13.0, 16.0, 20.0, 25.0, 30.0, 40.0, |
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.
See internal discussion - please sync with Surbhi on the default buckets we use.
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.
[Update] |
93fa217
to
d6e2f11
Compare
d6e2f11
to
8f3dbc3
Compare
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.
@rahul2393 I'm not a Googler, so sorry for my external review.
I made a few comments about the code style, but it nits.
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.
LGTM.
020802d
to
9d8ee5e
Compare
7d37c41
to
da6b8ce
Compare
fa026aa
to
1dceb29
Compare
# Conflicts: # spanner/go.mod # spanner/test/opentelemetry/test/go.mod
This PR adds built-in metrics to the Go spanner client library. This will introduce below metrics for the consumers of this library .
This feature is not currently available for customers to use. Once GA'ed consumers of this library will start getting these metrics in the MetricsExplorer