Implement Telemetry Metrics for Metadata Server #2616

muhamadazmy · 2025-02-03T16:06:14Z

Implement Telemetry Metrics for Metadata Server

Summary:
This pull request introduces telemetry metrics to the metadata server, enhancing monitoring and performance analysis capabilities.

Fixes #2605

muhamadazmy · 2025-02-03T16:18:26Z

@tillrohrmann feel free to suggest more metrics to add that you think can be useful.

tillrohrmann

Thanks for adding metrics to the RaftMetadataStore @muhamadazmy. The changes look good to me. What we might also add as metrics are the applied index, committed index, snapshot size, trim index, leader, term, etc. (basically what we report in MetadataStoreSummary). This could be done as a follow-up if you want to merge this PR.

tillrohrmann · 2025-02-03T20:26:14Z

It would be great to investigate why the e2e test is failing. I don't expect that your changes are causing it but we need to harden these test failures and make sure that it is not pointing towards a real problem.

muhamadazmy · 2025-02-04T07:53:58Z

@tillrohrmann Great! thank you for your review. I wanted to add some of those metrics but wanted to make sure they would be useful since they already reported in the summary.

I will investigate why the tests are failing.

muhamadazmy · 2025-02-04T13:36:28Z

Thank you @tillrohrmann for your review. I added more metrics as you suggested. I also added few simple metrics to the metadata store client as well as Ahmed suggested.

AhmedSoliman · 2025-02-04T09:14:04Z

crates/metadata-server/src/grpc/handler.rs

 use restate_core::metadata_store::{serialize_value, Precondition};
 use restate_types::config::Configuration;
 use restate_types::metadata_store::keys::NODES_CONFIG_KEY;
 use restate_types::nodes_config::NodesConfiguration;
 use restate_types::storage::StorageCodec;
 use std::ops::Deref;
+use std::time::Instant;


Can we use tokio's Instant, thanks :)

AhmedSoliman · 2025-02-04T17:17:19Z

crates/core/src/metadata_store.rs

@@ -299,28 +306,43 @@ impl MetadataStoreClient {
        &self,
        key: ByteString,
    ) -> Result<Option<T>, ReadError> {
-        let value = self.inner.get(key).await?;
+        let start_time = Instant::now();
+        counter!(METADATA_CLIENT_GET_TOTAL, "key" => key.to_string()).increment(1);


I'm a little worried about the cardinality of this counter. We have a key per partition for epoch metadata.

Yeah, i wasn't sure about this tbh. On other hand it also can help debugging irregular access pattern to a certain key.

What about if the key is prefixed with pp_epoch we only use that prefix for the metrics?

it's okay, let's leave the key for counters, but histogram per key is not great.

AhmedSoliman · 2025-02-04T17:18:25Z

crates/core/src/metadata_store.rs

@@ -26,6 +32,7 @@ use restate_types::storage::{StorageCodec, StorageDecode, StorageEncode, Storage
 use restate_types::{flexbuffers_storage_encode_decode, Version, Versioned};
 use std::future::Future;
 use std::sync::Arc;
+use std::time::Instant;


In general, it's better to use tokio's Instant, but that's not an issue in this case. Just a reminder to form the habit.

AhmedSoliman · 2025-02-04T17:19:38Z

crates/core/src/metadata_store.rs

+        counter!(METADATA_CLIENT_GET_VERSION_TOTAL, "key" => key.to_string()).increment(1);
+        let result = self.inner.get_version(key.clone()).await;
+
+        histogram!(METADATA_CLIENT_GET_VERSION_DURATION, "key" => key.to_string())
+            .record(start_time.elapsed());


That's a lot of histograms (cardinality is a problem here as well)

Yeah, Indeed. How do you feel about the suggested solution above. Another solution of course it to drop the key label completely

AhmedSoliman · 2025-02-04T17:22:05Z

crates/metadata-server/src/grpc/handler.rs

@@ -53,6 +60,9 @@ impl MetadataStoreHandler {
 #[async_trait]
 impl MetadataServerSvc for MetadataStoreHandler {
    async fn get(&self, request: Request<GetRequest>) -> Result<Response<GetResponse>, Status> {
+        counter!(METADATA_SERVER_GET_TOTAL).increment(1);


it would have been nice if the counter has a dimension for status (success vs. failure). Imagine wanting to monitor success rate of a give metadata server, how would we do that?

This comment applies to all fallible operations.

Yeah, that's definitely a good idea. I will update it

AhmedSoliman · 2025-02-04T17:24:41Z

crates/metadata-server/src/metric_definitions.rs

+
+// Raft specific metrics
+pub(crate) const METADATA_SERVER_RAFT_SENT_MESSAGE_TOTAL: &str =
+    "restate.metadata_server.raft.sent_messages.total";


Do we need the raft part in this?

It's raft specific metric. But maybe should renamed to embedded or replicated instead since raft is just an implementation detail

muhamadazmy · 2025-02-05T10:58:19Z

Thanks @AhmedSoliman for your review. I have processed all your comments. I also dropped the key label from the client metrics, and recorded separate counter for both success and error for both client and server.

AhmedSoliman · 2025-02-05T11:34:03Z

Do we have per-key histograms on the server metrics?

AhmedSoliman · 2025-02-05T11:40:53Z

crates/core/src/metadata_store.rs

+            @success=METADATA_CLIENT_GET_TOTAL_SUCCESS,
+            @error=METADATA_CLIENT_GET_TOTAL_ERROR,
+            @duration=METADATA_CLIENT_GET_DURATION,


Separating successes and errors into different metric names makes hard at query time to aggregate. Maybe take a look at an example in task_center.rs that emits the status as dimension instead.

counter!(TC_FINISHED, "kind" => kind_str, "status" => TC_STATUS_FAILED) .increment(1);

or from ingress:

counter!( INGRESS_REQUESTS, "status" => REQUEST_COMPLETED, "rpc.service" => service_name, "rpc.method" => handler_name, ) .increment(1);

You are right! I should have thought of that :D

AhmedSoliman · 2025-02-05T11:42:44Z

crates/core/src/metadata_store.rs

+macro_rules! metrics_helper {
+    (@success=$ok:ident, @error=$err:ident, @duration=$dur:ident, $block:block) => {{
+        let start_time = tokio::time::Instant::now();
+        let result = $block;
+        metrics::histogram!($dur).record(start_time.elapsed());
+        if result.is_ok() {
+            metrics::counter!($ok).increment(1);
+        } else {
+            metrics::counter!($err).increment(1);
+        }
+
+        result
+    }};
+}
+


I'm not sure if it's worth a macro if this is used only a couple of times to be honest. I'm not sure if it needs a macro if it's just the dimension name that will report the status.

It's only to not repeat myself. It's easier to make changes for example.

AhmedSoliman · 2025-02-05T13:31:31Z

crates/metadata-server/src/metric_definitions.rs

+        "Raft Metadata raft term number"
+    );
+
+    describe_gauge!(
+        METADATA_SERVER_EMBEDDED_APPLIED_LSN,
+        Unit::Count,
+        "Raft Metadata raft applied lsn"
+    );
+
+    describe_gauge!(
+        METADATA_SERVER_EMBEDDED_COMMITTED_LSN,
+        Unit::Count,
+        "Raft Metadata raft committed lsn"
+    );
+
+    describe_gauge!(
+        METADATA_SERVER_EMBEDDED_FIRST_INDEX,
+        Unit::Count,
+        "Raft Metadata raft first index"
+    );
+    describe_gauge!(
+        METADATA_SERVER_EMBEDDED_LAST_INDEX,
+        Unit::Count,
+        "Raft Metadata raft last index"


embedded vs. raft vs. replicated? cc. @tillrohrmann

I've renamed the Raft metadata server kind to "replicated" in this PR: #2632.

AhmedSoliman · 2025-02-05T13:32:27Z

crates/metadata-server/src/raft/network/connection_manager.rs

+                                    counter!(METADATA_SERVER_EMBEDDED_RECV_MESSAGE_TOTAL, "peer" => self.remote_peer.to_string())
+                                        .increment(1);
+                                    counter!(METADATA_SERVER_EMBEDDED_RECV_MESSAGE_BYTES, "peer" => self.remote_peer.to_string())
+                                        .increment(message.payload.len() as u64);


Let's remove the peer address, this is literally infinite cardinality as those sockets get random ports

This is not the peer address, this only holds the plain node id. But if it doesn't add any value i can remove it.

If it's not the address, then it's okay.

AhmedSoliman · 2025-02-05T13:32:34Z

crates/metadata-server/src/raft/network/connection_manager.rs

+        counter!(METADATA_SERVER_EMBEDDED_SENT_MESSAGE_TOTAL, "peer" => self.remote_peer.to_string())
+            .increment(1);
+        counter!(METADATA_SERVER_EMBEDDED_SENT_MESSAGE_BYTES, "peer" => self.remote_peer.to_string())
+            .increment(size as u64);


Summary: This pull request introduces telemetry metrics to the metadata server, enhancing monitoring and performance analysis capabilities. Fixes restatedev#2605

muhamadazmy · 2025-02-05T17:27:42Z

@AhmedSoliman thank you for your helpful input. I processed all your comments. Feel free to take another look, leave comments or let me know if it's Approved :)

AhmedSoliman

Looks ready to go

muhamadazmy force-pushed the pr2616 branch from b366ecb to d0aefe4 Compare February 3, 2025 16:11

muhamadazmy changed the title ~~metadata server metrics~~ Implement Telemetry Metrics for Metadata Server Feb 3, 2025

muhamadazmy force-pushed the pr2616 branch from d0aefe4 to 8bec8a9 Compare February 3, 2025 16:13

muhamadazmy requested a review from tillrohrmann February 3, 2025 16:17

muhamadazmy marked this pull request as ready for review February 3, 2025 16:22

tillrohrmann approved these changes Feb 3, 2025

View reviewed changes

muhamadazmy force-pushed the pr2616 branch 2 times, most recently from 86d4c7d to bf1ecd6 Compare February 4, 2025 13:24

muhamadazmy requested a review from tillrohrmann February 4, 2025 13:35

AhmedSoliman reviewed Feb 4, 2025

View reviewed changes

muhamadazmy force-pushed the pr2616 branch from bf1ecd6 to 4cff35a Compare February 5, 2025 10:52

muhamadazmy requested a review from AhmedSoliman February 5, 2025 10:56

AhmedSoliman reviewed Feb 5, 2025

View reviewed changes

muhamadazmy force-pushed the pr2616 branch 4 times, most recently from 3f1d9b7 to 3a770d5 Compare February 5, 2025 12:57

muhamadazmy requested a review from AhmedSoliman February 5, 2025 12:59

AhmedSoliman reviewed Feb 5, 2025

View reviewed changes

Implement Telemetry Metrics for Metadata Server

cc6fc0f

Summary: This pull request introduces telemetry metrics to the metadata server, enhancing monitoring and performance analysis capabilities. Fixes restatedev#2605

muhamadazmy force-pushed the pr2616 branch from 3a770d5 to cc6fc0f Compare February 5, 2025 17:04

AhmedSoliman approved these changes Feb 5, 2025

View reviewed changes

muhamadazmy merged commit 2760587 into restatedev:main Feb 5, 2025
54 checks passed

muhamadazmy deleted the pr2616 branch February 5, 2025 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Telemetry Metrics for Metadata Server #2616

Implement Telemetry Metrics for Metadata Server #2616

muhamadazmy commented Feb 3, 2025 •

edited

Loading

muhamadazmy commented Feb 3, 2025

tillrohrmann left a comment

tillrohrmann commented Feb 3, 2025

muhamadazmy commented Feb 4, 2025

muhamadazmy commented Feb 4, 2025

AhmedSoliman Feb 4, 2025

AhmedSoliman Feb 4, 2025

muhamadazmy Feb 4, 2025

AhmedSoliman Feb 5, 2025

AhmedSoliman Feb 4, 2025

AhmedSoliman Feb 4, 2025

muhamadazmy Feb 4, 2025

AhmedSoliman Feb 4, 2025

muhamadazmy Feb 4, 2025

AhmedSoliman Feb 4, 2025

muhamadazmy Feb 4, 2025

muhamadazmy commented Feb 5, 2025

AhmedSoliman commented Feb 5, 2025

AhmedSoliman Feb 5, 2025

muhamadazmy Feb 5, 2025

AhmedSoliman Feb 5, 2025

muhamadazmy Feb 5, 2025

AhmedSoliman Feb 5, 2025

tillrohrmann Feb 5, 2025 •

edited

Loading

AhmedSoliman Feb 5, 2025

muhamadazmy Feb 5, 2025 •

edited

Loading

AhmedSoliman Feb 5, 2025

AhmedSoliman Feb 5, 2025

muhamadazmy commented Feb 5, 2025

AhmedSoliman left a comment

Implement Telemetry Metrics for Metadata Server #2616

Implement Telemetry Metrics for Metadata Server #2616

Conversation

muhamadazmy commented Feb 3, 2025 • edited Loading

muhamadazmy commented Feb 3, 2025

tillrohrmann left a comment

Choose a reason for hiding this comment

tillrohrmann commented Feb 3, 2025

muhamadazmy commented Feb 4, 2025

muhamadazmy commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhamadazmy commented Feb 5, 2025

AhmedSoliman commented Feb 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tillrohrmann Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhamadazmy Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhamadazmy commented Feb 5, 2025

AhmedSoliman left a comment

Choose a reason for hiding this comment

muhamadazmy commented Feb 3, 2025 •

edited

Loading

tillrohrmann Feb 5, 2025 •

edited

Loading

muhamadazmy Feb 5, 2025 •

edited

Loading