Skip to content

Conversation

mbissa
Copy link

@mbissa mbissa commented Sep 16, 2025

stats/otel: Part 1 for A94
Adds up down counter boiler plate code

RELEASE NOTES: none

@mbissa mbissa added this to the 1.76 Release milestone Sep 16, 2025
@mbissa mbissa requested a review from easwars September 16, 2025 07:11
@mbissa mbissa added the Type: Feature New features or improvements in behavior label Sep 16, 2025
@mbissa mbissa force-pushed the a94-subchannel-metrics branch from 20f43a0 to e4d9b0e Compare September 16, 2025 07:14
Copy link

codecov bot commented Sep 16, 2025

Codecov Report

❌ Patch coverage is 26.86567% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.48%. Comparing base (e60a04b) to head (e4d9b0e).
⚠️ Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
stats/opentelemetry/opentelemetry.go 4.00% 24 Missing ⚠️
internal/testutils/stats/test_metrics_recorder.go 28.57% 15 Missing ⚠️
internal/stats/metrics_recorder_list.go 0.00% 6 Missing ⚠️
experimental/stats/metricregistry.go 73.33% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8581      +/-   ##
==========================================
- Coverage   81.64%   80.48%   -1.17%     
==========================================
  Files         413      413              
  Lines       40621    40933     +312     
==========================================
- Hits        33167    32946     -221     
- Misses       5991     6286     +295     
- Partials     1463     1701     +238     
Files with missing lines Coverage Δ
experimental/stats/metrics.go 0.00% <ø> (ø)
experimental/stats/metricregistry.go 55.63% <73.33%> (-44.37%) ⬇️
internal/stats/metrics_recorder_list.go 83.33% <0.00%> (-16.67%) ⬇️
internal/testutils/stats/test_metrics_recorder.go 45.27% <28.57%> (-30.06%) ⬇️
stats/opentelemetry/opentelemetry.go 59.13% <4.00%> (-18.78%) ⬇️

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Pranjali-2501 Pranjali-2501 modified the milestones: 1.76 Release, 1.77 Sep 17, 2025
@easwars
Copy link
Contributor

easwars commented Sep 18, 2025

Please follow the instructions mentioned here and update the PR description: https://github.com/grpc/grpc-go/blob/master/CONTRIBUTING.md#pr-descriptions

Specifically the following:

Read and follow the guidelines for PR titles and descriptions here: https://google.github.io/eng-practices/review/developer/cl-descriptions.html

particularly the sections "First Line" and "Body is Informative".

@easwars easwars modified the milestone: 1.77 Release Sep 18, 2025
Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

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

LGTM

I'm completely new to the otel codebase in grpc-go though. So, hopefully a second reviewer knows more :)

recorder.RecordInt64Count(h, incr, labels...)
}

// Int64UpDownCountHandle is a typed handle for an int up-down counter metric.
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand this docstring is very similar to the existing one for Int64CountHandle. But I'm curious as to what the "typed handle" means here.

Copy link
Member

Choose a reason for hiding this comment

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

It's a newtype for MetricDescriptor - all different metric types have the same Descriptor information, but we give them all their own types so that they can be used correctly. (I.e. they have proper method signatures on Record.) The Register function returns a Handle, which is just a pointer, that it uses to access the metric's info in the Recorder. E.g. on line 111, the Record call passes the Handle to the recorder, which the recorder uses to look up the metric in a map.

}
ret, err := meter.Int64UpDownCounter(string(metricName), options...)
if err != nil {
logger.Errorf("failed to register metric \"%v\", will not record: %v", metricName, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Log statements should follow capitalization rules for regular sentences, and not the ones that apply to error strings. See: go/go-style/decisions#error-strings

Copy link
Member

Choose a reason for hiding this comment

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

Please address.

@easwars easwars removed their assignment Sep 18, 2025
@mbissa mbissa requested a review from dfawley September 23, 2025 09:18
recorder.RecordInt64Count(h, incr, labels...)
}

// Int64UpDownCountHandle is a typed handle for an int up-down counter metric.
Copy link
Member

Choose a reason for hiding this comment

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

It's a newtype for MetricDescriptor - all different metric types have the same Descriptor information, but we give them all their own types so that they can be used correctly. (I.e. they have proper method signatures on Record.) The Register function returns a Handle, which is just a pointer, that it uses to access the metric's info in the Recorder. E.g. on line 111, the Record call passes the Handle to the recorder, which the recorder uses to look up the metric in a map.


// Record records the int64 count value on the metrics recorder provided.
func (h *Int64CountHandle) Record(recorder MetricsRecorder, incr int64, labels ...string) {
recorder.RecordInt64Count(h, incr, labels...)
Copy link
Member

Choose a reason for hiding this comment

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

What happens if incr is negative here?

}
ret, err := meter.Int64UpDownCounter(string(metricName), options...)
if err != nil {
logger.Errorf("failed to register metric \"%v\", will not record: %v", metricName, err)
Copy link
Member

Choose a reason for hiding this comment

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

Please address.

@dfawley dfawley removed their assignment Oct 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants