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

grpc: Add StaticMethod CallOption #6926

Merged
merged 5 commits into from
Feb 15, 2024

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented Jan 18, 2024

This PR adds a call option that specifies whether a method is static. This is intended to be used by generated code and users to get a method attribute within OpenTelemetry while protecting the cardinality of OpenTelemetry timeseries.

RELEASE NOTES:

  • grpc: Add StaticMethod CallOption as a signal to stats handler that a method is safe to use as an instrument key.

@zasweq zasweq added the Type: Feature New features or improvements in behavior label Jan 18, 2024
@zasweq zasweq added this to the 1.61 Release milestone Jan 18, 2024
@zasweq zasweq requested a review from dfawley January 18, 2024 00:27
Copy link

codecov bot commented Jan 18, 2024

Codecov Report

Merging #6926 (9755a3d) into master (ddd377f) will decrease coverage by 1.25%.
Report is 25 commits behind head on master.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6926      +/-   ##
==========================================
- Coverage   83.51%   82.27%   -1.25%     
==========================================
  Files         287      296       +9     
  Lines       30920    31454     +534     
==========================================
+ Hits        25824    25878      +54     
- Misses       4020     4505     +485     
+ Partials     1076     1071       -5     
Files Coverage Δ
rpc_util.go 77.40% <0.00%> (-0.44%) ⬇️

... and 44 files with indirect coverage changes

@zasweq zasweq modified the milestones: 1.61 Release, 1.62 Release Jan 23, 2024
rpc_util.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned zasweq and unassigned dfawley Jan 25, 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.

Here's where to add the new version:

@zasweq
Copy link
Contributor Author

zasweq commented Feb 6, 2024

Added.

@zasweq
Copy link
Contributor Author

zasweq commented Feb 6, 2024

Let me know if you want to still change the name of this Call Option.

@ginayeh ginayeh modified the milestones: 1.62 Release, 1.63 Release Feb 8, 2024
rpc_util.go Outdated
// RegisteredMethod returns a CallOption which specifies that a call comes from
// a registered method. This does nothing functionally, but serves as a signal
// to other code.
func RegisteredMethod() CallOption {
Copy link
Member

Choose a reason for hiding this comment

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

StaticMethod, as discussed offline.

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.

@zasweq zasweq changed the title grpc: Add RegisteredMethodCallOption grpc: Add StaticMethodCallOption Feb 14, 2024
rpc_util.go Outdated
Comment on lines 202 to 205
type StaticMethodCallOption struct{}

func (StaticMethodCallOption) before(*callInfo) error { return nil }
func (StaticMethodCallOption) after(*callInfo, *csAttempt) {}
Copy link
Member

Choose a reason for hiding this comment

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

Let's do:

type StaticMethodCallOption struct {
	EmptyCallOption
}

(Then there is no need for before/after to be declared.)

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.

rpc_util.go Outdated
Comment on lines 193 to 194
// to a method that is static, which the method is known at compile time and
// doesn't change at runtime. This can be used as a signal to stats plugins that
Copy link
Member

Choose a reason for hiding this comment

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

"that is static, which means ..."?

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.

@dfawley dfawley assigned zasweq and unassigned dfawley Feb 14, 2024
@dfawley dfawley changed the title grpc: Add StaticMethodCallOption grpc: Add StaticMethod CallOption Feb 14, 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.

Please backport to the upcoming release branch ASAP.

@zasweq zasweq merged commit 29997a0 into grpc:master Feb 15, 2024
14 checks passed
zasweq added a commit to zasweq/grpc-go that referenced this pull request Feb 15, 2024
@dfawley dfawley modified the milestones: 1.63 Release, 1.62 Release Feb 15, 2024
zasweq added a commit that referenced this pull request Feb 15, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants