-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
gcp/observability: Add support for custom tags #5565
Conversation
gcp/observability/config.go
Outdated
@@ -41,6 +39,69 @@ const ( | |||
|
|||
var logFilterPatternRegexp = regexp.MustCompile(logFilterPatternRegexpStr) | |||
|
|||
// LogFilter represents a method logging configuration. | |||
type LogFilter struct { |
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 type should not be exported, right? Same below.
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.
Done.
gcp/observability/config.go
Outdated
// configuration is required for tracing/metrics/logging to function. This | ||
// config captures the most common knobs for gRPC users. It's always possible to | ||
// override with explicit config in code. | ||
type ObvConfig struct { |
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.
Just config
? Or maybe jsonConfig
?
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.
Chose config.
gcp/observability/opencensus.go
Outdated
@@ -38,26 +37,59 @@ var ( | |||
defaultMetricsReportingInterval = time.Second * 30 | |||
) | |||
|
|||
func convertTagsToMonitoringLabels(tags map[string]string) *stackdriver.Labels { |
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.
Drop the convert
; it's cleaner?
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.
Done.
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.
Thanks for the comments :D!
gcp/observability/config.go
Outdated
// configuration is required for tracing/metrics/logging to function. This | ||
// config captures the most common knobs for gRPC users. It's always possible to | ||
// override with explicit config in code. | ||
type ObvConfig struct { |
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.
Chose config.
gcp/observability/opencensus.go
Outdated
@@ -38,26 +37,59 @@ var ( | |||
defaultMetricsReportingInterval = time.Second * 30 | |||
) | |||
|
|||
func convertTagsToMonitoringLabels(tags map[string]string) *stackdriver.Labels { |
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.
Done.
This PR adds support for custom tags in Observability for Metrics, Tracing, and Logging. Custom tags are defined in the configuration and attached to emitted Logs, Traces, and Metrics. Currently there is no way to test this E2E, so the best we can do is verify the config passed into the Tracing and Metrics exporter has the correct custom tags present. Also, since this added a new field to the configuration, we decided it would be better if we got rid of the implementation detail of representation the configuration with a proto file, which I switched to an internal struct.
This PR does the following:
RELEASE NOTES: