[Issue #96] this is a PoC to solve Issue #96. #97
Conversation
|
hi @bradleyjkemp, how would you like to go forward with this issue and the PR? |
|
@marcellanz I'm so sorry this has been left unanswered for so long. It's been a while since I worked with gRPC day to day but your analysis + writeup in #96 is excellent. I've left a couple of comments (in particular I'm curious if this can be solved once during grpc-proxy startup) but this change looks good to me. It seems like it will only improve message dumping, I can't see it will worsen anything. |
|
|
||
| func (p *pbm) MarshalJSON() ([]byte, error) { | ||
| fd := make([]*desc.FileDescriptor, 0) | ||
| proto_descriptor.MsgDesc.Lock() |
There was a problem hiding this comment.
As proto_descriptor.MsgDesc is only going to be mutated during startup (i.e. when loading proto files), I wonder if this step of converting to creating a new dynamic.AnyResolver needs to be done every time?
Should (/could) this just be done once during proto file loading?
There was a problem hiding this comment.
Good point. Beside that, and as I mentioned in #96,
is probably not clean in the general way grpc-tools is designed
proto_descriptor.MsgDesc is a global variable and I'm not sure if this is the right way to bring these message describtors into the dump interceptor. It was the fastest way to have them be available, but should be probably done in a cleaner way. Wdyt?
Co-authored-by: Bradley Kemp <bradleyjkemp@users.noreply.github.com>
|
thanks @bradleyjkemp for getting back on this.
thanks @bradleyjkemp on getting back on this issue. grpc-tools have been very valuable for my work recently and I just ran a local binary for the time being. I'm looking forward to get the fix/feature into the repository so we can use it through a released version. I've left one comment above about how to share the message descriptors between the internal startup code and grpc-dump we might get cleaned up or not before merging the PR. |
Co-authored-by: Bradley Kemp <bradleyjkemp@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## master #97 +/- ##
==========================================
+ Coverage 56.49% 57.24% +0.74%
==========================================
Files 31 31
Lines 1177 1207 +30
==========================================
+ Hits 665 691 +26
- Misses 408 410 +2
- Partials 104 106 +2
Continue to review full report at Codecov.
|
|
Thanks for improving this! I'll release it now along with #106 |
This should fix issue #96.
Getting messages resolved using proto.MessageTypeand get then registered by proto.RegisterType is probably a bad idea and should only used by generated Go types by protoc.
After some debugging and tests I got with a solution where dumpInterceptor wraps a messages event into a type that implements json.Marshal interface and configures an AnyResolver for the jsonpb.Marshaler knowing all FileDescriptors the proto_descriptor.LoadProtoDirectories function ever sees during the load of the protofiles provided by the -proto_roots argument.
This is implemented in this PR and works for the use case mentioned at the beginning of this issue.
dump_001.json is a successful run of grpc-dump with this PR applied.
@bradleyjkemp I openend this as a Draft PR as my implementation is probably not clean in the general way grpc-tools is designed. I also do not know the protoreflect library well enough. Please let me know of any other way the issue described can be fixed or how you would integrate the functionality into
grpc-dumpproperly.