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

Support retrieve name of NamedService by instance #929

Closed
tisonkun opened this issue Feb 26, 2022 · 7 comments · Fixed by #930
Closed

Support retrieve name of NamedService by instance #929

tisonkun opened this issue Feb 26, 2022 · 7 comments · Fixed by #930

Comments

@tisonkun
Copy link
Contributor

tisonkun commented Feb 26, 2022

Feature Request

Crates

tonic

Motivation

When I'm trying to retrieve name of health service, it's impossible since the exact type consist an inner struct. Although I can still copy the name string literally and fallback to set_service_status interface, it would be reasonable to retrieve the name of NamedService by instance.

Proposal

I'll submit a patch and it should simply add a getter.

Alternatives

N/A

@LucioFranco
Copy link
Member

LucioFranco commented Mar 1, 2022

Could you explain more your use case and why the current way of getting the name doesn't work? I would imagine it still possible to use the const field from the trait without a method.

@tisonkun
Copy link
Contributor Author

tisonkun commented Mar 2, 2022

I would imagine it still possible

That would be nice. Could you share a code snippet how to?

So far, base on #930 I can write as below (src):

let (mut health_reporter, health_server) = tonic_health::server::health_reporter();
let health_server_name = health_server.name();

But I may not a Rust expert and don't know how to access the const field since HealthService is not exported.

@LucioFranco
Copy link
Member

Ah okay I see, I think the better route would be to just make HealthService public instead of adding a method to the trait. What do you think?

@tisonkun
Copy link
Contributor Author

tisonkun commented Mar 3, 2022

@LucioFranco that's OK for my use case. I'm curious why add a method is a bad idea.

@LucioFranco
Copy link
Member

@tisonkun its a duplicate of what you can already do and I'd like to avoid extra stuff we don't need :)

@tisonkun
Copy link
Contributor Author

tisonkun commented Mar 4, 2022

@LucioFranco make sense. Let me adjust the PR in this way.

@jctaoo
Copy link

jctaoo commented Aug 17, 2023

My usecase is to bind tonic service to axum router described at #1111 , code like:

    let grpc_route = axum::Router::new()
        .route(
            "/enhance_ws.RealtimeHandler/*rpc",
            axum::routing::any_service(RealtimeHandlerServer::new(grpc_service)),
        )
        .route(
            "/enhance_ws.webhook.WebhookScriptManage/*rpc",
            // format!(WebhookScriptManageServer::),
            axum::routing::any_service(WebhookScriptManageServer::new(web_hook_service)),
        )
        .layer(from_fn(|req, next| grpc_auth_layer(req, next, token)))
        .with_state(app_state.clone());

if it's possible to retrieve name of service, I can do something like:

.route(
            format!("{}/*rpc", WebhookScriptManageServer::name),
            axum::routing::any_service(WebhookScriptManageServer::new(web_hook_service)),
)

and it's also useful in binding dynamic service to route.

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

Successfully merging a pull request may close this issue.

3 participants