From 8174bf736241c32b55ade5fd42c3dd4d2933172b Mon Sep 17 00:00:00 2001 From: bstrzele Date: Mon, 23 Dec 2024 11:20:14 +0000 Subject: [PATCH] review fix --- src/metric_config.cpp | 2 +- src/model_metric_reporter.cpp | 23 ++++------------------- src/model_metric_reporter.hpp | 25 ++++++++----------------- src/test/metrics_flow_test.cpp | 10 ++++------ 4 files changed, 17 insertions(+), 43 deletions(-) diff --git a/src/metric_config.cpp b/src/metric_config.cpp index 7fe8830e48..200fded329 100644 --- a/src/metric_config.cpp +++ b/src/metric_config.cpp @@ -59,7 +59,7 @@ const std::string METRIC_NAME_REQUESTS_ACCEPTED = "ovms_requests_accepted"; const std::string METRIC_NAME_REQUESTS_REJECTED = "ovms_requests_rejected"; const std::string METRIC_NAME_GRAPH_ERROR = "ovms_graph_error"; -const std::string METRIC_NAME_PROCESSING_TIME = "ovms_processing_time"; +const std::string METRIC_NAME_PROCESSING_TIME = "ovms_graph_processing_time_us"; bool MetricConfig::validateEndpointPath(const std::string& endpoint) { std::regex valid_endpoint_regex("^/[a-zA-Z0-9]*$"); diff --git a/src/model_metric_reporter.cpp b/src/model_metric_reporter.cpp index 34bf93f63d..2b6c34c55a 100644 --- a/src/model_metric_reporter.cpp +++ b/src/model_metric_reporter.cpp @@ -564,38 +564,23 @@ MediapipeServableMetricReporter::MediapipeServableMetricReporter(const MetricCon // KFS this->processingTimeGrpcModelInfer = family->addMetric({{"name", graphName}, - {"api", "KServe"}, - {"method", "ModelInfer"}, - {"interface", "gRPC"}}, + {"method", "ModelInfer"}}, this->buckets); THROW_IF_NULL(this->processingTimeGrpcModelInfer, "cannot create metric"); this->processingTimeGrpcModelInferStream = family->addMetric({{"name", graphName}, - {"api", "KServe"}, - {"method", "ModelInferStream"}, - {"interface", "gRPC"}}, + {"method", "ModelInferStream"}}, this->buckets); THROW_IF_NULL(this->processingTimeGrpcModelInfer, "cannot create metric"); - this->processingTimeRestModelInfer = family->addMetric({{"name", graphName}, - {"api", "KServe"}, - {"method", "ModelInfer"}, - {"interface", "REST"}}, - this->buckets); - THROW_IF_NULL(this->processingTimeRestModelInfer, "cannot create metric"); - // V3 this->processingTimeRestV3Unary = family->addMetric({{"name", graphName}, - {"api", "V3"}, - {"method", "Unary"}, - {"interface", "REST"}}, + {"method", "Unary"}}, this->buckets); THROW_IF_NULL(this->processingTimeRestV3Unary, "cannot create metric"); this->processingTimeRestV3Stream = family->addMetric({{"name", graphName}, - {"api", "V3"}, - {"method", "Stream"}, - {"interface", "REST"}}, + {"method", "Stream"}}, this->buckets); THROW_IF_NULL(this->processingTimeRestV3Stream, "cannot create metric"); } diff --git a/src/model_metric_reporter.hpp b/src/model_metric_reporter.hpp index d820af7078..4146431f32 100644 --- a/src/model_metric_reporter.hpp +++ b/src/model_metric_reporter.hpp @@ -215,23 +215,14 @@ class MediapipeServableMetricReporter { std::unique_ptr processingTimeRestV3Stream; inline MetricHistogram* getProcessingTimeMetric(const ExecutionContext& context) { - if (context.interface == ExecutionContext::Interface::GRPC) { - if (context.method == ExecutionContext::Method::ModelInfer) - return this->processingTimeGrpcModelInfer.get(); - if (context.method == ExecutionContext::Method::ModelInferStream) - return this->processingTimeGrpcModelInferStream.get(); - return nullptr; - } else if (context.interface == ExecutionContext::Interface::REST) { - if (context.method == ExecutionContext::Method::ModelInfer) - return this->processingTimeRestModelInfer.get(); - if (context.method == ExecutionContext::Method::V3Unary) - return this->processingTimeRestV3Unary.get(); - if (context.method == ExecutionContext::Method::V3Stream) - return this->processingTimeRestV3Stream.get(); - return nullptr; - } else { - return nullptr; - } + if (context.method == ExecutionContext::Method::ModelInfer) + return this->processingTimeGrpcModelInfer.get(); + if (context.method == ExecutionContext::Method::ModelInferStream) + return this->processingTimeGrpcModelInferStream.get(); + if (context.method == ExecutionContext::Method::V3Unary) + return this->processingTimeRestV3Unary.get(); + if (context.method == ExecutionContext::Method::V3Stream) + return this->processingTimeRestV3Stream.get(); return nullptr; } diff --git a/src/test/metrics_flow_test.cpp b/src/test/metrics_flow_test.cpp index 70cf611775..45c20af3b0 100644 --- a/src/test/metrics_flow_test.cpp +++ b/src/test/metrics_flow_test.cpp @@ -396,8 +396,7 @@ TEST_F(MetricFlowTest, GrpcModelInfer) { checkMediapipeRequestsCounter(server.collect(), METRIC_NAME_RESPONSES, mpName, "gRPC", "ModelInfer", "KServe", numberOfAcceptedRequests); - EXPECT_THAT(server.collect(), HasSubstr(METRIC_NAME_PROCESSING_TIME + std::string{"_count{api=\"KServe\",interface=\"gRPC\",method=\"ModelInfer\",name=\""} + mpName + std::string{"\"} "} + std::to_string(numberOfAcceptedRequests))); - EXPECT_THAT(server.collect(), HasSubstr(METRIC_NAME_PROCESSING_TIME + std::string{"_count{api=\"KServe\",interface=\"REST\",method=\"ModelInfer\",name=\""} + mpName + std::string{"\"} "} + std::to_string(0))); + EXPECT_THAT(server.collect(), HasSubstr(METRIC_NAME_PROCESSING_TIME + std::string{"_count{method=\"ModelInfer\",name=\""} + mpName + std::string{"\"} "} + std::to_string(numberOfAcceptedRequests))); #endif EXPECT_THAT(server.collect(), HasSubstr(METRIC_NAME_REQUEST_TIME + std::string{"_count{interface=\"gRPC\",name=\""} + modelName + std::string{"\",version=\"1\"} "} + std::to_string(numberOfSuccessRequests))); @@ -712,8 +711,7 @@ TEST_F(MetricFlowTest, RestModelInfer) { checkMediapipeRequestsCounter(server.collect(), METRIC_NAME_RESPONSES, mpName, "REST", "ModelInfer", "KServe", numberOfAcceptedRequests); - EXPECT_THAT(server.collect(), HasSubstr(METRIC_NAME_PROCESSING_TIME + std::string{"_count{api=\"KServe\",interface=\"gRPC\",method=\"ModelInfer\",name=\""} + mpName + std::string{"\"} "} + std::to_string(0))); - EXPECT_THAT(server.collect(), HasSubstr(METRIC_NAME_PROCESSING_TIME + std::string{"_count{api=\"KServe\",interface=\"REST\",method=\"ModelInfer\",name=\""} + mpName + std::string{"\"} "} + std::to_string(numberOfAcceptedRequests))); + EXPECT_THAT(server.collect(), HasSubstr(METRIC_NAME_PROCESSING_TIME + std::string{"_count{method=\"ModelInfer\",name=\""} + mpName + std::string{"\"} "} + std::to_string(numberOfAcceptedRequests))); #endif EXPECT_THAT(server.collect(), HasSubstr(METRIC_NAME_REQUEST_TIME + std::string{"_count{interface=\"gRPC\",name=\""} + modelName + std::string{"\",version=\"1\"} "} + std::to_string(0))); @@ -835,7 +833,7 @@ TEST_F(MetricFlowTest, RestV3Unary) { // checkMediapipeRequestsCounter(server.collect(), METRIC_NAME_REQUESTS_REJECTED, "dummy_gpt", "REST", "Unary", "V3", numberOfRejectedRequests); checkMediapipeRequestsCounter(server.collect(), METRIC_NAME_RESPONSES, "dummy_gpt", "REST", "Unary", "V3", numberOfAcceptedRequests * 2); - EXPECT_THAT(server.collect(), HasSubstr(METRIC_NAME_PROCESSING_TIME + std::string{"_count{api=\"V3\",interface=\"REST\",method=\"Unary\",name=\""} + "dummy_gpt" + std::string{"\"} "} + std::to_string(numberOfAcceptedRequests * 2))); + EXPECT_THAT(server.collect(), HasSubstr(METRIC_NAME_PROCESSING_TIME + std::string{"_count{method=\"Unary\",name=\""} + "dummy_gpt" + std::string{"\"} "} + std::to_string(numberOfAcceptedRequests * 2))); } #endif @@ -889,7 +887,7 @@ TEST_F(MetricFlowTest, RestV3Stream) { const int numberOfMockedChunksPerRequest = 9; // Defined in openai_chat_completions_mock_calculator.cpp checkMediapipeRequestsCounter(server.collect(), METRIC_NAME_RESPONSES, "dummy_gpt", "REST", "Stream", "V3", numberOfAcceptedRequests * numberOfMockedChunksPerRequest * 2); - EXPECT_THAT(server.collect(), HasSubstr(METRIC_NAME_PROCESSING_TIME + std::string{"_count{api=\"V3\",interface=\"REST\",method=\"Stream\",name=\""} + "dummy_gpt" + std::string{"\"} "} + std::to_string(numberOfAcceptedRequests * 2))); + EXPECT_THAT(server.collect(), HasSubstr(METRIC_NAME_PROCESSING_TIME + std::string{"_count{method=\"Stream\",name=\""} + "dummy_gpt" + std::string{"\"} "} + std::to_string(numberOfAcceptedRequests * 2))); SPDLOG_ERROR(server.collect()); }