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 Method Descriptor Exposure #55

Closed
artificial-aidan opened this issue Dec 19, 2023 · 7 comments
Closed

GRPC Method Descriptor Exposure #55

artificial-aidan opened this issue Dec 19, 2023 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@artificial-aidan
Copy link

artificial-aidan commented Dec 19, 2023

I'm working on some features in which we are attaching custom options to grpc methods. This can be exposed with something like this:

for method_proto in svc_desc_proto.method:
            method_name = method_proto.name
            method_desc: MethodDescriptor = service_descriptor.methods_by_name[method_name]

            options = method_desc.GetOptions()

I had started putting together a PR that exposed the MethodOptions output of the GetOptions call, but then I realized that maybe it just makes sense to put the MethodDescriptor in the MethodMetadata instead of pulling out individual fields. This would allow for advanced usage if needed, and not require upkeep.

And I guess while at it we could put the ServiceDescriptor somewhere as well. And probably the FieldDescriptors.

But the MethodDescriptor had the most obvious place to store it.

class MethodMetaData(NamedTuple):
    input_type: Any
    output_type: Any
    method_type: MethodType
    handler: Any
    descriptor: MethodDescriptor

metadata[method_name] = MethodMetaData(
    method_type=method_type,
    input_type=input_type,
    output_type=output_type,
    handler=handler,
    descriptor=method_desc
)
@ViridianForge
Copy link
Collaborator

That does sound like it makes a good bit of sense - would perhaps allow making some of my own hamfisted attempts to open up access to fields information a bit more elegant.

If you wanted to put up a PR, that'd be much appreciated - for my own part, I'm going to be pretty swamped until after the holidays with my day to day.

@ViridianForge ViridianForge added the enhancement New feature or request label Dec 22, 2023
@ViridianForge ViridianForge self-assigned this Dec 26, 2023
@ViridianForge
Copy link
Collaborator

You're very correct @artificial-aidan - the method descriptor exposure is pretty straight forward.

I'm going to work on a bit of a middle ground on the service and file descriptors, and add them to the base client in an accessible manner, at least for this pass.

@artificial-aidan
Copy link
Author

Sounds good to me. Do you still want me to put something together for method descriptors?

I think a nice middle ground of providing the basic functionality, while exposing the descriptor protos is a good place to be. If you are leaving the simplicity that this library has created (which is awesome) then maybe you just use the raw protos.

@ViridianForge
Copy link
Collaborator

I have got the method descriptors started, just need to write tests and get it up - should have some time to finish it up in the next day or two.

@ViridianForge
Copy link
Collaborator

#56
Sorry that took a bit - this ought to expose the MethodDescriptors a bit more cleanly.

When it comes to the Service and File Descriptors - I think that could be accomplished to some degree without major changes by making the currently private _get_file_descriptor_by_symbol method public and adding a convenience method around self._desc_pool.FindServiceByName(name) (and adding usage examples).

Though, rather than tacking that on to this PR - I figured I'd bounce it off of you to see if that idea would match your use case.

@artificial-aidan
Copy link
Author

That looks great. And exposing the get_ methods for service and file descriptors would work fine. Allows for advanced usage if needed, and simple usage too.

@ViridianForge
Copy link
Collaborator

ViridianForge commented Jan 2, 2024

Thanks! Those enhancements will be tracked in Issue 57. Once those are merged to develop, we'll get version 0.1.14 up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants