-
Notifications
You must be signed in to change notification settings - Fork 26
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
Added gRPC telemetry instrumentation example. #163
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey, thanks for your contribution! a few nits here and there but else it looks good! could you also run cargo fmt
so the formatting is all good? ill then run the CI on it and merge it if it passes
Sure,
I suspect the JetBrains default code formatter ran over all files instead
of rust fmt since I didn’t updated the project settings.
Let fix those small things once I’m back in the office and I issue an
update probably over the weekend.
Thanks for your review and suggestions.
M
…On Fri, Dec 1, 2023 at 8:15 PM Mari ***@***.***> wrote:
***@***.**** commented on this pull request.
hey, thanks for your contribution! a few nits here and there but else it
looks good! could you also run cargo fmt so the formatting is all good?
ill then run the CI on it and merge it if it passes
------------------------------
In examples/grpc-http/src/server.rs
<#163 (comment)>
:
> +#[tonic::async_trait]
+impl JobRunner for MyJobRunner {
+ #[autometrics]
as you want to instrument every function in this impl block, you can use
#[autometrics] on the whole block:
⬇️ Suggested change
-#[tonic::async_trait]
-impl JobRunner for MyJobRunner {
- #[autometrics]
+#[autometrics] // note that autometrics needs to come BEFORE async_trait
+#[tonic::async_trait]
+impl JobRunner for MyJobRunner {
------------------------------
In examples/grpc-http/src/server.rs
<#163 (comment)>
:
> +impl JobRunner for MyJobRunner {
+ #[autometrics]
+ async fn send_job(&self, request: Request<JobRequest>) -> Result<Response<JobReply>, Status> {
+ println!("Got a request: {:?}", request);
+
+ // Write into the mock database
+ self.db_manager.write_into_table().await.expect("Failed to query database");
+
+ let reply = job::JobReply {
+ message: format!("Hello {}!", request.into_inner().name).into(),
+ };
+
+ Ok(Response::new(reply))
+ }
+
+ #[autometrics]
⬇️ Suggested change
- #[autometrics]
------------------------------
In examples/README.md
<#163 (comment)>
:
> @@ -14,6 +14,7 @@ cargo run --package example-{name of example}
- [custom-metrics](./custom-metrics/) - Define your own custom metrics alongside the ones generated by autometrics (using any of the metrics collection crates)
- [exemplars-tracing](./exemplars-tracing/) - Use fields from `tracing::Span`s as Prometheus exemplars
- [opentelemetry-push](./opentelemetry-push/) - Push metrics to an OpenTelemetry Collector via the OTLP HTTP or gRPC protocol using the Autometrics provided interface
+- [grpc-http](./grpc-http/) - Instrument Rust gRPC services with observability using Tonic, warp, and Autometrics.
⬇️ Suggested change
-- [grpc-http](./grpc-http/) - Instrument Rust gRPC services with observability using Tonic, warp, and Autometrics.
+- [grpc-http](./grpc-http/) - Instrument Rust gRPC services with metrics using Tonic, warp, and Autometrics.
------------------------------
In examples/grpc-http/README.md
<#163 (comment)>
:
> +In a second terminal, run
+
+```bash
+ ps | grep grpc-http
+```
+
+Sample output:
+
+```
+73014 ttys002 0:00.25 /Users/.../autometrics-rs/target/debug/grpc-http
+```
+
+In this example, the service runs on PID 73014. Let's send a sigterm signal to shutdown the service. On you system, a different PID will be returned so please use that one instead.
+
+```bash
+Kill 73014
⬇️ Suggested change
-Kill 73014
+kill 73014
------------------------------
In examples/grpc-http/src/main.rs
<#163 (comment)>
:
> + .recv()
+ .await;
+
+ // Shutdown the DB connection.
+ dbm.close_db().await.expect("Failed to close database connection");
+
+ println!("gRPC shutdown complete");
+}
+
+fn print_start(web_addr: &SocketAddr, grpc_addr: &SocketAddr) {
+ println!();
+ println!("Started gRPC server on port {:?}", grpc_addr.port());
+ println!("Started metrics on port {:?}", web_addr.port());
+ println!("Explore autometrics at http://127.0.0.1:6789");
+ println!();
+}
can u add a newline at the end of the file
------------------------------
In examples/grpc-http/Cargo.toml
<#163 (comment)>
:
> +[package]
+name = "grpc-http"
+version = "0.0.0"
+publish = false
+edition = "2021"
+
+[dependencies]
+autometrics = { path = "../../autometrics", features = ["prometheus-exporter"] }
+prost = "0.12"
+tokio = { version = "1", features = ["full"] }
+tonic = "0.10"
+tonic-health = "0.10"
+warp = "0.3"
+
+[build-dependencies]
+tonic-build = "0.10"
can u add a newline at the end of this file as well
—
Reply to this email directly, view it on GitHub
<#163 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFYR7XE57J7GX22NCYBB64LYHHC5NAVCNFSM6AAAAABACKHU66VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONJZGY3TGMRXGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
… SIGINT, SIGTERM, SIGQUIT and that works on Windows, Linux, and Mac. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Ok, I added the fixes you mentioned, improved the signal handler to work on all major operating systems, Please review again and, if no objection, feel free to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last small nit but else looks good
This doesn't compile. Please make those changes yourself as I don't have
the time to investigate conflicting macro implementations.
Thanks.
…On Mon, Dec 4, 2023 at 7:55 PM Mari ***@***.***> wrote:
***@***.**** approved this pull request.
last small nit but else looks good
------------------------------
In examples/grpc-http/src/server.rs
<#163 (comment)>
:
> +#[tonic::async_trait]
+#[autometrics(objective = API_SLO)]
can u swap those two around
⬇️ Suggested change
-#[tonic::async_trait]
-#[autometrics(objective = API_SLO)]
+#[autometrics(objective = API_SLO)]
+#[tonic::async_trait]
—
Reply to this email directly, view it on GitHub
<#163 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFYR7XHOFMFSOGLSG2ER55TYHW24JAVCNFSM6AAAAABACKHU66VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONRSGIZDQOBVHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Resolves issue: #162
Added end-to-end example to instrument gRPC service with graceful shutdown and health service for K8s deployment.
Updated example Readme.