fix(server/v2/grpc): fix reflection #23333
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the gRPC server reflection implementation in the Changes
Sequence DiagramsequenceDiagram
participant Client
participant ServerReflection
participant ServiceRegistry
Client->>ServerReflection: Request service reflection
ServerReflection->>ServiceRegistry: Find services for messages
ServiceRegistry-->>ServerReflection: Return service descriptors
ServerReflection-->>Client: Provide reflection information
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/v2/api/grpc/gogoreflection/serverreflection.go (2)
461-491: Add documentation and improve error handling.While the implementation is correct, consider these improvements:
- Add documentation explaining the message-based service lookup strategy.
- Enhance error messages with more context about why the descriptor or service lookup failed.
+// getServices returns a unique list of services and their file descriptors based on the provided messages. +// It looks up each message in the registry and finds associated services by scanning method input/output types. func (s *serverReflectionServer) getServices(messages []string) (svcs []string, fds []*dpb.FileDescriptorProto) { registry, err := gogoproto.MergedRegistry() if err != nil { - s.log.Error("unable to load merged registry", "err", err) + s.log.Error("failed to load protobuf registry for reflection", "err", err) return nil, nil } seenSvc := map[protoreflect.FullName]struct{}{} for _, messageName := range messages { md, err := registry.FindDescriptorByName(protoreflect.FullName(messageName)) if err != nil { - s.log.Error("unable to load message descriptor", "message", messageName, "err", err) + s.log.Error("failed to find message descriptor in registry", + "message", messageName, + "err", err, + "hint", "ensure the message is properly registered") continue }
493-517: Add documentation and improve return values.The implementation is efficient, but consider these improvements:
- Add documentation explaining the function's purpose and return values.
- Consider returning (protoreflect.ServiceDescriptor, error) instead of a boolean for more idiomatic Go.
+// findServiceForMessage searches the registry for a service that uses the given message +// as either an input or output type in any of its methods. It returns the first matching +// service descriptor and true if found, or a nil descriptor and false if not found. func findServiceForMessage(registry *protoregistry.Files, messageDesc protoreflect.MessageDescriptor) (protoreflect.ServiceDescriptor, bool) { var ( service protoreflect.ServiceDescriptor found bool )Alternative implementation with error return:
-func findServiceForMessage(registry *protoregistry.Files, messageDesc protoreflect.MessageDescriptor) (protoreflect.ServiceDescriptor, bool) { +func findServiceForMessage(registry *protoregistry.Files, messageDesc protoreflect.MessageDescriptor) (protoreflect.ServiceDescriptor, error) { var service protoreflect.ServiceDescriptor registry.RangeFiles(func(fileDescriptor protoreflect.FileDescriptor) bool { // ... existing implementation ... }) - return service, found + if service == nil { + return nil, fmt.Errorf("no service found for message %s", messageDesc.FullName()) + } + return service, nil }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/v2/api/grpc/gogoreflection/serverreflection.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/api/grpc/gogoreflection/serverreflection.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (3)
server/v2/api/grpc/gogoreflection/serverreflection.go (3)
57-57: LGTM!The addition of the protoregistry import is necessary for the new message-based reflection implementation.
66-66: LGTM!The field rename from
methodstomessagesaccurately reflects the shift to message-based reflection.
75-80: LGTM!The Register function changes are consistent with the shift to message-based reflection, and the logger is properly initialized.
|
I will migrate this test: as a system test, as it was testing reflection but only for v0 apps. |
Adding the backport label only backporting the test changes |
julienrbrt
added
the
backport/v0.52.x
github-actions
bot
added
C:server/v2
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
tests/systemtests/grpc_test.go (2)
22-24: Consider making the gRPC endpoint configurable.The gRPC endpoint (
localhost:9090) is hardcoded. Consider making it configurable through environment variables or test configuration.+const ( + defaultGRPCHost = "localhost" + defaultGRPCPort = "9090" +) + ctx := context.Background() -grpcClient, err := grpc.NewClient("localhost:9090", grpc.WithTransportCredentials(insecure.NewCredentials())) +host := getEnvOrDefault("GRPC_HOST", defaultGRPCHost) +port := getEnvOrDefault("GRPC_PORT", defaultGRPCPort) +grpcClient, err := grpc.NewClient(fmt.Sprintf("%s:%s", host, port), grpc.WithTransportCredentials(insecure.NewCredentials()))
26-30: Consider adding more comprehensive service checks.The test only verifies one service. Consider adding checks for other critical services that should be available through reflection.
server/v2/api/grpc/gogoreflection/serverreflection.go (3)
461-489: Consider adding metrics for reflection service health.The error handling is good, but consider adding metrics to track:
- Number of failed message descriptor lookups
- Number of services not found for messages
- Total number of successful reflections
493-517: Consider optimizing service lookup performance.The
findServiceForMessagefunction iterates through all files and services. For large service registries, this could be inefficient. Consider:
- Caching service-to-message mappings
- Using a more efficient lookup structure
+type serviceCache struct { + sync.RWMutex + messageToService map[protoreflect.FullName]protoreflect.ServiceDescriptor +} + +func (s *serverReflectionServer) buildServiceCache(registry *protoregistry.Files) { + s.cache.Lock() + defer s.cache.Unlock() + + registry.RangeFiles(func(fd protoreflect.FileDescriptor) bool { + // Build cache + return true + }) +}
506-510: Consider adding debug logging for service matching.Adding debug logs here would help troubleshoot when messages don't match with any service.
if methodDesc.Input() == messageDesc || methodDesc.Output() == messageDesc { + s.log.Debug("found service for message", + "message", messageDesc.FullName(), + "service", serviceDesc.FullName(), + "method", methodDesc.FullName()) service = serviceDesc found = true return false }CHANGELOG.md (3)
Line range hint
1-1: Add title to the CHANGELOG fileThe CHANGELOG file should start with a clear title like "# Changelog" to follow standard changelog conventions.
+ # Changelog
Line range hint
44-48: Consider adding migration guide linksFor major breaking changes sections, consider adding links to migration guides to help users upgrade smoothly.
Line range hint
2960-2965: Add link to current changelogThe reference to previous changelog versions should include a link to the current changelog location for better navigation.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/systemtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
CHANGELOG.md(2 hunks)server/v2/api/grpc/gogoreflection/serverreflection.go(5 hunks)tests/integration/runtime/query_test.go(0 hunks)tests/systemtests/go.mod(6 hunks)tests/systemtests/grpc_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/runtime/query_test.go
🧰 Additional context used
📓 Path-based instructions (4)
tests/systemtests/grpc_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/systemtests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/api/grpc/gogoreflection/serverreflection.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (4)
tests/systemtests/grpc_test.go (1)
23-23: Review security implications of using insecure credentials.The test uses
insecure.NewCredentials(). While this might be acceptable for testing, ensure this aligns with your security requirements.tests/systemtests/go.mod (1)
9-9: LGTM! Dependencies look appropriate.The added dependencies align well with the gRPC reflection testing requirements.
Also applies to: 30-30, 37-37, 67-68, 106-106, 126-126, 171-171
server/v2/api/grpc/gogoreflection/serverreflection.go (1)
66-66: LGTM! Field rename reflects the new message-based approach.The rename from
methodstomessagesbetter represents the new message-based reflection approach.CHANGELOG.md (1)
Line range hint
1-3000: LGTM on overall changelog structure and contentThe changelog follows good practices by:
- Organizing changes by type (Features, Bug Fixes, etc.)
- Including version numbers and dates
- Documenting breaking changes clearly
- Providing detailed descriptions of changes
| require.NoError(t, err) | ||
| require.Greater(t, len(services), 0) | ||
| require.Contains(t, services, "cosmos.staking.v1beta1.Query") | ||
| } |
There was a problem hiding this comment.
Shall add a query invocation in this test?
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit