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

stats: Various CSM Observability bug fixes #7278

Merged
merged 2 commits into from
May 29, 2024
Merged

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented May 29, 2024

This PR fixes various bugs I found while interop testing CSM Observability.

  1. "service_namespace" -> "service_namespace_name
  2. OpenTelemetry component default metrics were not working properly
  3. Have a version as a separate call

RELEASE NOTES: N/A

@zasweq zasweq requested a review from dfawley May 29, 2024 21:10
@zasweq zasweq added this to the 1.65 Release milestone May 29, 2024
stats/opentelemetry/client_metrics.go Show resolved Hide resolved
@@ -44,12 +44,15 @@ func (h *clientStatsHandler) initializeMetrics() {
return
}

meter := h.o.MetricsOptions.MeterProvider.Meter("grpc-go " + grpc.Version)
meter := h.o.MetricsOptions.MeterProvider.Meter("grpc-go ", otelmetric.WithInstrumentationVersion(grpc.Version))
Copy link
Member

Choose a reason for hiding this comment

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

Delete this space in the string and the one below please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 51 to 54
setOfMetrics := DefaultMetrics.metrics
if h.o.MetricsOptions.Metrics != nil {
setOfMetrics = h.o.MetricsOptions.Metrics.metrics
}
Copy link
Member

Choose a reason for hiding this comment

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

Go style prefers:

setOfMetrics := h.o.MetricsOptions.Metrics
if setOfMetrics == nil {
	setOfMetrics = DefaultMetrics.metrics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Note that this conditional is on the .Metrics field so switched to metrics.metrics in the create calls.

@dfawley dfawley assigned zasweq and unassigned dfawley May 29, 2024
@zasweq zasweq assigned dfawley and unassigned zasweq May 29, 2024
@dfawley dfawley assigned zasweq and unassigned dfawley May 29, 2024
@zasweq zasweq merged commit 0756c0d into grpc:master May 29, 2024
11 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants