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/opentelemetry: Cleanup OpenTelemetry API's before stabilization #7874

Merged
merged 4 commits into from
Dec 2, 2024

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented Nov 25, 2024

This PR cleans up some OpenTelemetry API's before stabilization.

RELEASE NOTES:

  • stats/opentelemetry: Introduce new APIs to enable OpenTelemetry instrumentation for metrics on servers and clients

@zasweq zasweq requested a review from dfawley November 25, 2024 23:47
@zasweq zasweq added the Type: Documentation Documentation or examples label Nov 25, 2024
@zasweq zasweq added this to the 1.69 Release milestone Nov 25, 2024
Copy link

codecov bot commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 82.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 82.12%. Comparing base (967ba46) to head (7f99944).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
stats/metrics.go 70.96% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7874      +/-   ##
==========================================
+ Coverage   81.94%   82.12%   +0.17%     
==========================================
  Files         377      377              
  Lines       38120    38112       -8     
==========================================
+ Hits        31239    31298      +59     
+ Misses       5572     5520      -52     
+ Partials     1309     1294      -15     
Files with missing lines Coverage Δ
experimental/stats/metricregistry.go 100.00% <100.00%> (ø)
internal/testutils/stats/test_metrics_recorder.go 74.00% <100.00%> (ø)
stats/opentelemetry/client_metrics.go 87.93% <ø> (ø)
stats/opentelemetry/opentelemetry.go 75.86% <100.00%> (ø)
stats/opentelemetry/server_metrics.go 89.37% <ø> (ø)
stats/metrics.go 70.96% <70.96%> (ø)

... and 25 files with indirect coverage changes

@dfawley dfawley added Type: Feature New features or improvements in behavior and removed Type: Documentation Documentation or examples labels Nov 26, 2024
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor wording suggestions. Please check out the edits-as-commits and suggested changes.

Comment on lines 77 to 82
// cardinality and could cause severe memory or performance problems. On
// Client Side, to record the method name along with metric instead of
// bucketing into "other pass a grpc.StaticMethodCallOption as a call option
// into Invoke or NewStream.
//
// This only applies for server side metrics.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// cardinality and could cause severe memory or performance problems. On
// Client Side, to record the method name along with metric instead of
// bucketing into "other pass a grpc.StaticMethodCallOption as a call option
// into Invoke or NewStream.
//
// This only applies for server side metrics.
// cardinality and could cause severe memory or performance problems.
//
// This only applies for server-side metrics. For clients, to record
// the method name in the attributes, pass grpc.StaticMethodCallOption
// to Invoke or NewStream. Note that when using protobuf generated
// clients, this CallOption is included automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done.

Comment on lines 85 to 86
// OptionalLabels specifies a list of optional labels that will be included
// on any metrics that have corresponding optional labels configured.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// OptionalLabels specifies a list of optional labels that will be included
// on any metrics that have corresponding optional labels configured.
// OptionalLabels specifies a list of optional labels to enable on any
// metrics that support them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I couldn't figure out a way to tersely convey it's just the subset that are "supported" that are enabled. (i.e. it's not always the full set added optionally, but the ones that are actually also configured. Changed to your suggestion.

@dfawley dfawley assigned zasweq and unassigned dfawley Nov 26, 2024
@zasweq zasweq force-pushed the o11y-release-documentation branch from 93e5272 to 7f99944 Compare November 26, 2024 23:06
@zasweq zasweq merged commit 3c0586a into grpc:master Dec 2, 2024
14 of 15 checks passed
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.

2 participants