From f730a0701b725e25e400292dfe484f1a6af80bcb Mon Sep 17 00:00:00 2001 From: tanvi-jagtap Date: Wed, 17 Apr 2024 16:29:19 +0000 Subject: [PATCH 01/26] gRFC for gpr logging to absl logging --- ...placing-gpr-logging-with-abseil-logging.md | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 L115-replacing-gpr-logging-with-abseil-logging.md diff --git a/L115-replacing-gpr-logging-with-abseil-logging.md b/L115-replacing-gpr-logging-with-abseil-logging.md new file mode 100644 index 000000000..8eda2f711 --- /dev/null +++ b/L115-replacing-gpr-logging-with-abseil-logging.md @@ -0,0 +1,54 @@ +L115: Replacing gpr logging with Abseil logging +---- + +* Author(s): tjagtap@google.com +* Approver: roth@google.com , ctiller@google.com +* Status: In Review +* Implemented in: gRPC C++ +* Last updated: April 18, 2024. +* Discussion at: (filled after thread exists) + +## Abstract + +Replacing gpr logging and asserts with Abseil logging and asserts + +## Background & Rationale + +gRPC is currently maintaining its own version of the logging and assert APIs. The Abseil library released their logs and asserts too. We want to leverage the Abseil library and avoid maintaining our own logging code. Also, the current implementation of gpr_log uses format strings which are not type safe and have exploit potential. Moving to absl logging will eliminate this security risk. + +## Related Proposals: + +None. + +## Proposal + +We are proposing to remove all instances of gpr logging and asserts and replace them with their absl equivalents. + +## Implementation + +### Reference +* gpr logging header : https://github.com/grpc/grpc/blame/83a17ff4684dc1fb3493a151ac0b655b1c55e766/include/grpc/support/log.h +* absl logging header : https://github.com/abseil/abseil-cpp/blob/master/absl/log/log.h +* absl assert header : https://github.com/abseil/abseil-cpp/blob/master/absl/log/check.h + +### Functions that will be removed, and their replacements +* `gpr_log` + * `gpr_log(GPR_DEBUG,...)` with `VLOG(2) << ...` + * `gpr_log(GPR_INFO,...)` with `LOG(INFO) < ...` + * `gpr_log(GPR_ERROR,...)` with `LOG(ERROR) < ...` +* `gpr_log_message` +` * `gpr_log_message(GPR_DEBUG,...)` with `VLOG(2) << ...` +` * `gpr_log_message(GPR_INFO,...)` with `LOG(INFO) < ...` +` * `gpr_log_message(GPR_ERROR,...)` with `LOG(ERROR) < ...` +* Asserts + * `GPR_ASSERT(...)` with `CHECK(...)` + * `GPR_DEBUG_ASSERT(...)` with `DCHECK(...)` +* `gpr_assertion_failed` with `CHECK(...)` +* `gpr_set_log_function` - This function will be removed. Teams can consider usage of absl AddLogSink. + +Functions that will be removed +* `gpr_log_severity_string` - This wont be needed anymore. +* `gpr_should_log` - This wont be needed anymore. + +Functions that will work as before +* `gpr_set_log_verbosity` - This will set the absl verbosity internally. From 4c5a3ad819e219fb483641c10955a24c1a455676 Mon Sep 17 00:00:00 2001 From: tanvi-jagtap Date: Wed, 17 Apr 2024 16:35:39 +0000 Subject: [PATCH 02/26] Fixing typo --- L115-replacing-gpr-logging-with-abseil-logging.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/L115-replacing-gpr-logging-with-abseil-logging.md b/L115-replacing-gpr-logging-with-abseil-logging.md index 8eda2f711..aa7172438 100644 --- a/L115-replacing-gpr-logging-with-abseil-logging.md +++ b/L115-replacing-gpr-logging-with-abseil-logging.md @@ -37,9 +37,9 @@ We are proposing to remove all instances of gpr logging and asserts and replace * `gpr_log(GPR_INFO,...)` with `LOG(INFO) < ...` * `gpr_log(GPR_ERROR,...)` with `LOG(ERROR) < ...` * `gpr_log_message` -` * `gpr_log_message(GPR_DEBUG,...)` with `VLOG(2) << ...` -` * `gpr_log_message(GPR_INFO,...)` with `LOG(INFO) < ...` -` * `gpr_log_message(GPR_ERROR,...)` with `LOG(ERROR) < ...` + * `gpr_log_message(GPR_DEBUG,...)` with `VLOG(2) << ...` + * `gpr_log_message(GPR_INFO,...)` with `LOG(INFO) < ...` + * `gpr_log_message(GPR_ERROR,...)` with `LOG(ERROR) < ...` * Asserts * `GPR_ASSERT(...)` with `CHECK(...)` * `GPR_DEBUG_ASSERT(...)` with `DCHECK(...)` From f05ebe0ee792845a15dbfa9637a8c8f8886f43c5 Mon Sep 17 00:00:00 2001 From: tanvi-jagtap Date: Wed, 17 Apr 2024 16:42:18 +0000 Subject: [PATCH 03/26] Fixing typo --- L115-replacing-gpr-logging-with-abseil-logging.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/L115-replacing-gpr-logging-with-abseil-logging.md b/L115-replacing-gpr-logging-with-abseil-logging.md index aa7172438..e0c54c34d 100644 --- a/L115-replacing-gpr-logging-with-abseil-logging.md +++ b/L115-replacing-gpr-logging-with-abseil-logging.md @@ -44,11 +44,13 @@ We are proposing to remove all instances of gpr logging and asserts and replace * `GPR_ASSERT(...)` with `CHECK(...)` * `GPR_DEBUG_ASSERT(...)` with `DCHECK(...)` * `gpr_assertion_failed` with `CHECK(...)` -* `gpr_set_log_function` - This function will be removed. Teams can consider usage of absl AddLogSink. +* `gpr_set_log_function` - This function will be removed. Teams can consider usage of LogSink and AddLogSink. + * LogSink https://github.com/abseil/abseil-cpp/blob/fa57bfc573453d57a38552eedcce894b0e2d9f5e/absl/log/log_sink.h + * AddLogSink https://github.com/abseil/abseil-cpp/blob/fa57bfc573453d57a38552eedcce894b0e2d9f5e/absl/log/log_sink_registry.h -Functions that will be removed +### Functions that will be removed * `gpr_log_severity_string` - This wont be needed anymore. * `gpr_should_log` - This wont be needed anymore. -Functions that will work as before +### Functions that will work as before * `gpr_set_log_verbosity` - This will set the absl verbosity internally. From 5bc40215a46f0ec2096cb30e1283da23e43c8505 Mon Sep 17 00:00:00 2001 From: tanvi-jagtap Date: Wed, 17 Apr 2024 16:46:27 +0000 Subject: [PATCH 04/26] Fixing the links --- L115-replacing-gpr-logging-with-abseil-logging.md | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/L115-replacing-gpr-logging-with-abseil-logging.md b/L115-replacing-gpr-logging-with-abseil-logging.md index e0c54c34d..13bb51c91 100644 --- a/L115-replacing-gpr-logging-with-abseil-logging.md +++ b/L115-replacing-gpr-logging-with-abseil-logging.md @@ -26,10 +26,10 @@ We are proposing to remove all instances of gpr logging and asserts and replace ## Implementation -### Reference -* gpr logging header : https://github.com/grpc/grpc/blame/83a17ff4684dc1fb3493a151ac0b655b1c55e766/include/grpc/support/log.h -* absl logging header : https://github.com/abseil/abseil-cpp/blob/master/absl/log/log.h -* absl assert header : https://github.com/abseil/abseil-cpp/blob/master/absl/log/check.h +### References +* [gpr logging header](https://github.com/grpc/grpc/blame/83a17ff4684dc1fb3493a151ac0b655b1c55e766/include/grpc/support/log.h) +* [absl logging header](https://github.com/abseil/abseil-cpp/blob/master/absl/log/log.h) +* [absl assert header](https://github.com/abseil/abseil-cpp/blob/master/absl/log/check.h) ### Functions that will be removed, and their replacements * `gpr_log` @@ -44,9 +44,7 @@ We are proposing to remove all instances of gpr logging and asserts and replace * `GPR_ASSERT(...)` with `CHECK(...)` * `GPR_DEBUG_ASSERT(...)` with `DCHECK(...)` * `gpr_assertion_failed` with `CHECK(...)` -* `gpr_set_log_function` - This function will be removed. Teams can consider usage of LogSink and AddLogSink. - * LogSink https://github.com/abseil/abseil-cpp/blob/fa57bfc573453d57a38552eedcce894b0e2d9f5e/absl/log/log_sink.h - * AddLogSink https://github.com/abseil/abseil-cpp/blob/fa57bfc573453d57a38552eedcce894b0e2d9f5e/absl/log/log_sink_registry.h +* `gpr_set_log_function` - This function will be removed. Teams can consider usage of [LogSink](https://github.com/abseil/abseil-cpp/blob/fa57bfc573453d57a38552eedcce894b0e2d9f5e/absl/log/log_sink.h) and [AddLogSink](https://github.com/abseil/abseil-cpp/blob/fa57bfc573453d57a38552eedcce894b0e2d9f5e/absl/log/log_sink_registry.h). ### Functions that will be removed * `gpr_log_severity_string` - This wont be needed anymore. From d942b4901fb2279dfb29bcf8f209289301604030 Mon Sep 17 00:00:00 2001 From: tanvi-jagtap Date: Fri, 19 Apr 2024 10:11:19 +0000 Subject: [PATCH 05/26] Adding details about verbosity --- L115-replacing-gpr-logging-with-abseil-logging.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/L115-replacing-gpr-logging-with-abseil-logging.md b/L115-replacing-gpr-logging-with-abseil-logging.md index 13bb51c91..6322c696b 100644 --- a/L115-replacing-gpr-logging-with-abseil-logging.md +++ b/L115-replacing-gpr-logging-with-abseil-logging.md @@ -50,5 +50,6 @@ We are proposing to remove all instances of gpr logging and asserts and replace * `gpr_log_severity_string` - This wont be needed anymore. * `gpr_should_log` - This wont be needed anymore. -### Functions that will work as before +### Will work as before +* `GRPC_VERBOSITY` - environment variable will continue to work as before if it is set. * `gpr_set_log_verbosity` - This will set the absl verbosity internally. From 4f85d1bb0842cb3384ea41fbef5e2405f2e3f033 Mon Sep 17 00:00:00 2001 From: tanvi-jagtap Date: Fri, 19 Apr 2024 10:58:42 +0000 Subject: [PATCH 06/26] Adding proposed behaviour --- ...placing-gpr-logging-with-abseil-logging.md | 47 +++++++++++++++++-- 1 file changed, 43 insertions(+), 4 deletions(-) diff --git a/L115-replacing-gpr-logging-with-abseil-logging.md b/L115-replacing-gpr-logging-with-abseil-logging.md index 6322c696b..557cd44fb 100644 --- a/L115-replacing-gpr-logging-with-abseil-logging.md +++ b/L115-replacing-gpr-logging-with-abseil-logging.md @@ -1,4 +1,4 @@ -L115: Replacing gpr logging with Abseil logging +L116: Replacing gpr logging with Abseil logging ---- * Author(s): tjagtap@google.com @@ -50,6 +50,45 @@ We are proposing to remove all instances of gpr logging and asserts and replace * `gpr_log_severity_string` - This wont be needed anymore. * `gpr_should_log` - This wont be needed anymore. -### Will work as before -* `GRPC_VERBOSITY` - environment variable will continue to work as before if it is set. -* `gpr_set_log_verbosity` - This will set the absl verbosity internally. +### Will work similar to before +* `gpr_set_log_verbosity` and GRPC_VERBOSITY will work as follows + +``` +void SomeInitFunctionCalledByGrpcInit() { + optional verbosity = GetEnv("GRPC_VERBOSITY"); + if (verbosity.has_value()) { + if (verbosity == "GPR_INFO" || verbosity == "INFO") { + SetMinLogLevel(INFO); + } + else if (verbosity == "GPR_ERROR" || verbosity == "ERROR") { + SetMinLogLevel(ERROR); + } + else if (verbosity == "WARNING") { + SetMinLogLevel(WARNING); + } + else if (verbosity == "GPR_DEBUG") { + SetGlobalVLogLevel(2); + SetMinLogLevel(INFO); + } + else { + LOG(ERROR) << "Unknown log verbosity: " << verbosity; + } + } +} + +void gpr_set_log_verbosity(gpr_log_severity verbosity) { + if (verbosity == GPR_INFO) { + SetMinLogLevel(INFO); + } + else if (verbosity == GPR_ERROR) { + SetMinLogLevel(ERROR); + } + else if (verbosity == GPR_DEBUG) { + SetGlobalVLogLevel(2); + SetMinLogLevel(INFO); + } + else { + LOG(ERROR) << "Unknown log verbosity: " << verbosity; + } +} +``` \ No newline at end of file From f61ba638ff6e437fab40452406440de6eccdfd1f Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Wed, 24 Apr 2024 16:43:32 +0530 Subject: [PATCH 07/26] Rename L115-replacing-gpr-logging-with-abseil-logging.md to L116-replacing-gpr-logging-with-abseil-logging.md --- ...ging.md => L116-replacing-gpr-logging-with-abseil-logging.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename L115-replacing-gpr-logging-with-abseil-logging.md => L116-replacing-gpr-logging-with-abseil-logging.md (99%) diff --git a/L115-replacing-gpr-logging-with-abseil-logging.md b/L116-replacing-gpr-logging-with-abseil-logging.md similarity index 99% rename from L115-replacing-gpr-logging-with-abseil-logging.md rename to L116-replacing-gpr-logging-with-abseil-logging.md index 557cd44fb..200f54962 100644 --- a/L115-replacing-gpr-logging-with-abseil-logging.md +++ b/L116-replacing-gpr-logging-with-abseil-logging.md @@ -91,4 +91,4 @@ void gpr_set_log_verbosity(gpr_log_severity verbosity) { LOG(ERROR) << "Unknown log verbosity: " << verbosity; } } -``` \ No newline at end of file +``` From 09a2281daa6c04c9d8cf34611f826518e53e4229 Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Thu, 25 Apr 2024 09:58:00 +0530 Subject: [PATCH 08/26] Rename L116-replacing-gpr-logging-with-abseil-logging.md to L116-cpp-replacing-gpr-logging-with-abseil-logging.md --- ...ng.md => L116-cpp-replacing-gpr-logging-with-abseil-logging.md | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename L116-replacing-gpr-logging-with-abseil-logging.md => L116-cpp-replacing-gpr-logging-with-abseil-logging.md (100%) diff --git a/L116-replacing-gpr-logging-with-abseil-logging.md b/L116-cpp-replacing-gpr-logging-with-abseil-logging.md similarity index 100% rename from L116-replacing-gpr-logging-with-abseil-logging.md rename to L116-cpp-replacing-gpr-logging-with-abseil-logging.md From 3cc42fcf8c690659ff298b13c6d4b58a386f9fb6 Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Thu, 25 Apr 2024 09:58:54 +0530 Subject: [PATCH 09/26] Update L116-cpp-replacing-gpr-logging-with-abseil-logging.md --- L116-cpp-replacing-gpr-logging-with-abseil-logging.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/L116-cpp-replacing-gpr-logging-with-abseil-logging.md b/L116-cpp-replacing-gpr-logging-with-abseil-logging.md index 200f54962..6c16895d2 100644 --- a/L116-cpp-replacing-gpr-logging-with-abseil-logging.md +++ b/L116-cpp-replacing-gpr-logging-with-abseil-logging.md @@ -3,7 +3,7 @@ L116: Replacing gpr logging with Abseil logging * Author(s): tjagtap@google.com * Approver: roth@google.com , ctiller@google.com -* Status: In Review +* Status: Draft * Implemented in: gRPC C++ * Last updated: April 18, 2024. * Discussion at: (filled after thread exists) From 55a71a472993650d9a3c527ad02319edf9cc03c7 Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Mon, 29 Apr 2024 10:31:05 +0530 Subject: [PATCH 10/26] Rename L116-cpp-replacing-gpr-logging-with-abseil-logging.md to L117-cpp-replacing-gpr-logging-with-abseil-logging.md --- ...ng.md => L117-cpp-replacing-gpr-logging-with-abseil-logging.md | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename L116-cpp-replacing-gpr-logging-with-abseil-logging.md => L117-cpp-replacing-gpr-logging-with-abseil-logging.md (100%) diff --git a/L116-cpp-replacing-gpr-logging-with-abseil-logging.md b/L117-cpp-replacing-gpr-logging-with-abseil-logging.md similarity index 100% rename from L116-cpp-replacing-gpr-logging-with-abseil-logging.md rename to L117-cpp-replacing-gpr-logging-with-abseil-logging.md From 3eaca8283be579ae48bef019d82e8165539b2401 Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Mon, 29 Apr 2024 10:31:23 +0530 Subject: [PATCH 11/26] Update L117-cpp-replacing-gpr-logging-with-abseil-logging.md --- L117-cpp-replacing-gpr-logging-with-abseil-logging.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/L117-cpp-replacing-gpr-logging-with-abseil-logging.md b/L117-cpp-replacing-gpr-logging-with-abseil-logging.md index 6c16895d2..fb7b80263 100644 --- a/L117-cpp-replacing-gpr-logging-with-abseil-logging.md +++ b/L117-cpp-replacing-gpr-logging-with-abseil-logging.md @@ -1,4 +1,4 @@ -L116: Replacing gpr logging with Abseil logging +L117: Replacing gpr logging with Abseil logging ---- * Author(s): tjagtap@google.com From 5280e29e7b1e04263bbb741c3b50ee0606e03988 Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Mon, 29 Apr 2024 10:31:43 +0530 Subject: [PATCH 12/26] Update L117-cpp-replacing-gpr-logging-with-abseil-logging.md --- L117-cpp-replacing-gpr-logging-with-abseil-logging.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/L117-cpp-replacing-gpr-logging-with-abseil-logging.md b/L117-cpp-replacing-gpr-logging-with-abseil-logging.md index fb7b80263..e21eeb72b 100644 --- a/L117-cpp-replacing-gpr-logging-with-abseil-logging.md +++ b/L117-cpp-replacing-gpr-logging-with-abseil-logging.md @@ -1,4 +1,4 @@ -L117: Replacing gpr logging with Abseil logging +Replacing gpr logging with Abseil logging ---- * Author(s): tjagtap@google.com From 54444c65685e92545010f83b2ea898173e1c6850 Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Mon, 29 Apr 2024 10:49:27 +0530 Subject: [PATCH 13/26] Update L117-cpp-replacing-gpr-logging-with-abseil-logging.md --- L117-cpp-replacing-gpr-logging-with-abseil-logging.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/L117-cpp-replacing-gpr-logging-with-abseil-logging.md b/L117-cpp-replacing-gpr-logging-with-abseil-logging.md index e21eeb72b..211e4eed2 100644 --- a/L117-cpp-replacing-gpr-logging-with-abseil-logging.md +++ b/L117-cpp-replacing-gpr-logging-with-abseil-logging.md @@ -34,12 +34,12 @@ We are proposing to remove all instances of gpr logging and asserts and replace ### Functions that will be removed, and their replacements * `gpr_log` * `gpr_log(GPR_DEBUG,...)` with `VLOG(2) << ...` - * `gpr_log(GPR_INFO,...)` with `LOG(INFO) < ...` - * `gpr_log(GPR_ERROR,...)` with `LOG(ERROR) < ...` + * `gpr_log(GPR_INFO,...)` with `LOG(INFO) << ...` + * `gpr_log(GPR_ERROR,...)` with `LOG(ERROR) << ...` * `gpr_log_message` * `gpr_log_message(GPR_DEBUG,...)` with `VLOG(2) << ...` - * `gpr_log_message(GPR_INFO,...)` with `LOG(INFO) < ...` - * `gpr_log_message(GPR_ERROR,...)` with `LOG(ERROR) < ...` + * `gpr_log_message(GPR_INFO,...)` with `LOG(INFO) << ...` + * `gpr_log_message(GPR_ERROR,...)` with `LOG(ERROR) << ...` * Asserts * `GPR_ASSERT(...)` with `CHECK(...)` * `GPR_DEBUG_ASSERT(...)` with `DCHECK(...)` From b188df338ceae342dd502ded0b0a3c965d913391 Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Mon, 29 Apr 2024 11:16:49 +0530 Subject: [PATCH 14/26] Update L117-cpp-replacing-gpr-logging-with-abseil-logging.md --- L117-cpp-replacing-gpr-logging-with-abseil-logging.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/L117-cpp-replacing-gpr-logging-with-abseil-logging.md b/L117-cpp-replacing-gpr-logging-with-abseil-logging.md index 211e4eed2..c751ad8da 100644 --- a/L117-cpp-replacing-gpr-logging-with-abseil-logging.md +++ b/L117-cpp-replacing-gpr-logging-with-abseil-logging.md @@ -1,4 +1,4 @@ -Replacing gpr logging with Abseil logging +L117-Replacing gpr logging with Abseil logging ---- * Author(s): tjagtap@google.com From 517c373a52642cd5181df7376496cf0ca5d3092b Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Mon, 29 Apr 2024 11:17:45 +0530 Subject: [PATCH 15/26] Update L117-cpp-replacing-gpr-logging-with-abseil-logging.md --- L117-cpp-replacing-gpr-logging-with-abseil-logging.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/L117-cpp-replacing-gpr-logging-with-abseil-logging.md b/L117-cpp-replacing-gpr-logging-with-abseil-logging.md index c751ad8da..d5da35394 100644 --- a/L117-cpp-replacing-gpr-logging-with-abseil-logging.md +++ b/L117-cpp-replacing-gpr-logging-with-abseil-logging.md @@ -3,9 +3,9 @@ L117-Replacing gpr logging with Abseil logging * Author(s): tjagtap@google.com * Approver: roth@google.com , ctiller@google.com -* Status: Draft +* Status: In Review * Implemented in: gRPC C++ -* Last updated: April 18, 2024. +* Last updated: April 29, 2024. * Discussion at: (filled after thread exists) ## Abstract From e2aba84f45e1c5822bf2a459e07f42e50520677f Mon Sep 17 00:00:00 2001 From: tanvi-jagtap Date: Mon, 6 May 2024 09:44:05 +0000 Subject: [PATCH 16/26] Fixed half the review comments --- ...placing-gpr-logging-with-abseil-logging.md | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/L117-cpp-replacing-gpr-logging-with-abseil-logging.md b/L117-cpp-replacing-gpr-logging-with-abseil-logging.md index d5da35394..98c907067 100644 --- a/L117-cpp-replacing-gpr-logging-with-abseil-logging.md +++ b/L117-cpp-replacing-gpr-logging-with-abseil-logging.md @@ -1,12 +1,12 @@ -L117-Replacing gpr logging with Abseil logging +L117: C-core: Replace gpr Logging with absl Logging ---- -* Author(s): tjagtap@google.com -* Approver: roth@google.com , ctiller@google.com +* Author(s): @tanvi-jagtap +* Approver: @markdroth , @ctiller * Status: In Review * Implemented in: gRPC C++ -* Last updated: April 29, 2024. -* Discussion at: (filled after thread exists) +* Last updated: 2024-05-06. +* Discussion at: https://groups.google.com/g/grpc-io/c/Jg7bvHqAyCU ## Abstract @@ -16,6 +16,11 @@ Replacing gpr logging and asserts with Abseil logging and asserts gRPC is currently maintaining its own version of the logging and assert APIs. The Abseil library released their logs and asserts too. We want to leverage the Abseil library and avoid maintaining our own logging code. Also, the current implementation of gpr_log uses format strings which are not type safe and have exploit potential. Moving to absl logging will eliminate this security risk. +### References +* [gpr logging header](https://github.com/grpc/grpc/blame/83a17ff4684dc1fb3493a151ac0b655b1c55e766/include/grpc/support/log.h) +* [absl logging header](https://github.com/abseil/abseil-cpp/blob/master/absl/log/log.h) +* [absl assert header](https://github.com/abseil/abseil-cpp/blob/master/absl/log/check.h) + ## Related Proposals: None. @@ -24,13 +29,6 @@ None. We are proposing to remove all instances of gpr logging and asserts and replace them with their absl equivalents. -## Implementation - -### References -* [gpr logging header](https://github.com/grpc/grpc/blame/83a17ff4684dc1fb3493a151ac0b655b1c55e766/include/grpc/support/log.h) -* [absl logging header](https://github.com/abseil/abseil-cpp/blob/master/absl/log/log.h) -* [absl assert header](https://github.com/abseil/abseil-cpp/blob/master/absl/log/check.h) - ### Functions that will be removed, and their replacements * `gpr_log` * `gpr_log(GPR_DEBUG,...)` with `VLOG(2) << ...` @@ -53,6 +51,8 @@ We are proposing to remove all instances of gpr logging and asserts and replace ### Will work similar to before * `gpr_set_log_verbosity` and GRPC_VERBOSITY will work as follows +## Implementation + ``` void SomeInitFunctionCalledByGrpcInit() { optional verbosity = GetEnv("GRPC_VERBOSITY"); From ffa0b8d7f32398f9b8eca83f73eabfc5cadb8e32 Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Mon, 6 May 2024 15:21:45 +0530 Subject: [PATCH 17/26] Rename L117-cpp-replacing-gpr-logging-with-abseil-logging.md to L117-core-replace-gpr-logging-with-abseil-logging.md --- ...ing.md => L117-core-replace-gpr-logging-with-abseil-logging.md | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename L117-cpp-replacing-gpr-logging-with-abseil-logging.md => L117-core-replace-gpr-logging-with-abseil-logging.md (100%) diff --git a/L117-cpp-replacing-gpr-logging-with-abseil-logging.md b/L117-core-replace-gpr-logging-with-abseil-logging.md similarity index 100% rename from L117-cpp-replacing-gpr-logging-with-abseil-logging.md rename to L117-core-replace-gpr-logging-with-abseil-logging.md From 9a3e55332e6e30b5940636c8823148bceb99a06e Mon Sep 17 00:00:00 2001 From: tanvi-jagtap Date: Mon, 6 May 2024 10:09:42 +0000 Subject: [PATCH 18/26] Fixed 1 review comment --- L117-core-replace-gpr-logging-with-abseil-logging.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/L117-core-replace-gpr-logging-with-abseil-logging.md b/L117-core-replace-gpr-logging-with-abseil-logging.md index 98c907067..3005339cf 100644 --- a/L117-core-replace-gpr-logging-with-abseil-logging.md +++ b/L117-core-replace-gpr-logging-with-abseil-logging.md @@ -51,8 +51,6 @@ We are proposing to remove all instances of gpr logging and asserts and replace ### Will work similar to before * `gpr_set_log_verbosity` and GRPC_VERBOSITY will work as follows -## Implementation - ``` void SomeInitFunctionCalledByGrpcInit() { optional verbosity = GetEnv("GRPC_VERBOSITY"); @@ -92,3 +90,9 @@ void gpr_set_log_verbosity(gpr_log_severity verbosity) { } } ``` + +## Implementation + + +* [GPR_ASSERT and GPR_DEBUG_ASSERT removal](https://github.com/grpc/grpc/pulls?q=%22%5Bgrpc%5D%5BGpr_To_Absl_Logging%5D+Migrating+from+gpr+to+absl+logging+GPR_ASSERT%22+) +* [gpr_log removal](https://github.com/grpc/grpc/pulls?q=%22%5Bgrpc%5D%5BGpr_To_Absl_Logging%5D+Migrating+from+gpr+to+absl+logging+-+gpr_log%22+) From 330f69d2bfa0df98ecf3a6b758bdff4373d3ce3b Mon Sep 17 00:00:00 2001 From: tanvi-jagtap Date: Tue, 7 May 2024 05:08:00 +0000 Subject: [PATCH 19/26] Edit rationale section --- L117-core-replace-gpr-logging-with-abseil-logging.md | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/L117-core-replace-gpr-logging-with-abseil-logging.md b/L117-core-replace-gpr-logging-with-abseil-logging.md index 3005339cf..c3e40694e 100644 --- a/L117-core-replace-gpr-logging-with-abseil-logging.md +++ b/L117-core-replace-gpr-logging-with-abseil-logging.md @@ -5,18 +5,19 @@ L117: C-core: Replace gpr Logging with absl Logging * Approver: @markdroth , @ctiller * Status: In Review * Implemented in: gRPC C++ -* Last updated: 2024-05-06. +* Last updated: 2024-05-07. * Discussion at: https://groups.google.com/g/grpc-io/c/Jg7bvHqAyCU ## Abstract Replacing gpr logging and asserts with Abseil logging and asserts -## Background & Rationale +## Background gRPC is currently maintaining its own version of the logging and assert APIs. The Abseil library released their logs and asserts too. We want to leverage the Abseil library and avoid maintaining our own logging code. Also, the current implementation of gpr_log uses format strings which are not type safe and have exploit potential. Moving to absl logging will eliminate this security risk. ### References + * [gpr logging header](https://github.com/grpc/grpc/blame/83a17ff4684dc1fb3493a151ac0b655b1c55e766/include/grpc/support/log.h) * [absl logging header](https://github.com/abseil/abseil-cpp/blob/master/absl/log/log.h) * [absl assert header](https://github.com/abseil/abseil-cpp/blob/master/absl/log/check.h) @@ -91,8 +92,13 @@ void gpr_set_log_verbosity(gpr_log_severity verbosity) { } ``` -## Implementation +## Rationale +* Format specifiers are not type-safe. We want to avoid these. +* We want to avoid maintaining platform specific gpr APIS when absl is providing them. +* Abseil provides a range of APIs such as PLOG, VLOG, DVLOG with specific logging frequencies such as LOG_IF, LOG_EVERY_N, LOG_EVERY_POW_2 etc. + +## Implementation * [GPR_ASSERT and GPR_DEBUG_ASSERT removal](https://github.com/grpc/grpc/pulls?q=%22%5Bgrpc%5D%5BGpr_To_Absl_Logging%5D+Migrating+from+gpr+to+absl+logging+GPR_ASSERT%22+) * [gpr_log removal](https://github.com/grpc/grpc/pulls?q=%22%5Bgrpc%5D%5BGpr_To_Absl_Logging%5D+Migrating+from+gpr+to+absl+logging+-+gpr_log%22+) From c8a4e8aa1c1ff3c4751e31c003e9e836c3b82e03 Mon Sep 17 00:00:00 2001 From: tanvi-jagtap Date: Tue, 7 May 2024 05:13:38 +0000 Subject: [PATCH 20/26] Edit rationale section --- L117-core-replace-gpr-logging-with-abseil-logging.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/L117-core-replace-gpr-logging-with-abseil-logging.md b/L117-core-replace-gpr-logging-with-abseil-logging.md index c3e40694e..42e55da5b 100644 --- a/L117-core-replace-gpr-logging-with-abseil-logging.md +++ b/L117-core-replace-gpr-logging-with-abseil-logging.md @@ -94,10 +94,17 @@ void gpr_set_log_verbosity(gpr_log_severity verbosity) { ## Rationale +### Advantages + * Format specifiers are not type-safe. We want to avoid these. * We want to avoid maintaining platform specific gpr APIS when absl is providing them. * Abseil provides a range of APIs such as PLOG, VLOG, DVLOG with specific logging frequencies such as LOG_IF, LOG_EVERY_N, LOG_EVERY_POW_2 etc. +### Action Items + +* Coding using the above APIs will need to migrate to the newer APIs. +* The application will need to call absl::InitializeLog() itself, or else there will be a log message indicating that logging is happening before absl logging is initialized. If in the future absl fixes [#1656](https://github.com/abseil/abseil-cpp/issues/1656), then we could consider initializing absl logging in gRPC init. + ## Implementation * [GPR_ASSERT and GPR_DEBUG_ASSERT removal](https://github.com/grpc/grpc/pulls?q=%22%5Bgrpc%5D%5BGpr_To_Absl_Logging%5D+Migrating+from+gpr+to+absl+logging+GPR_ASSERT%22+) From 4a397af518bf8d26bd971ebde359a8fe069d0964 Mon Sep 17 00:00:00 2001 From: tanvi-jagtap Date: Sat, 11 May 2024 02:57:00 +0000 Subject: [PATCH 21/26] Fixing typo --- L117-core-replace-gpr-logging-with-abseil-logging.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/L117-core-replace-gpr-logging-with-abseil-logging.md b/L117-core-replace-gpr-logging-with-abseil-logging.md index 42e55da5b..6db82805b 100644 --- a/L117-core-replace-gpr-logging-with-abseil-logging.md +++ b/L117-core-replace-gpr-logging-with-abseil-logging.md @@ -97,7 +97,7 @@ void gpr_set_log_verbosity(gpr_log_severity verbosity) { ### Advantages * Format specifiers are not type-safe. We want to avoid these. -* We want to avoid maintaining platform specific gpr APIS when absl is providing them. +* We want to avoid maintaining platform specific gpr APIs when absl is providing them. * Abseil provides a range of APIs such as PLOG, VLOG, DVLOG with specific logging frequencies such as LOG_IF, LOG_EVERY_N, LOG_EVERY_POW_2 etc. ### Action Items From 607f1dc9c44651d63004e83f95e15edc48b25921 Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Mon, 13 May 2024 10:10:32 +0530 Subject: [PATCH 22/26] Update L117-core-replace-gpr-logging-with-abseil-logging.md --- L117-core-replace-gpr-logging-with-abseil-logging.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/L117-core-replace-gpr-logging-with-abseil-logging.md b/L117-core-replace-gpr-logging-with-abseil-logging.md index 6db82805b..3276eab03 100644 --- a/L117-core-replace-gpr-logging-with-abseil-logging.md +++ b/L117-core-replace-gpr-logging-with-abseil-logging.md @@ -56,16 +56,16 @@ We are proposing to remove all instances of gpr logging and asserts and replace void SomeInitFunctionCalledByGrpcInit() { optional verbosity = GetEnv("GRPC_VERBOSITY"); if (verbosity.has_value()) { - if (verbosity == "GPR_INFO" || verbosity == "INFO") { + if (verbosity == "INFO") { SetMinLogLevel(INFO); } - else if (verbosity == "GPR_ERROR" || verbosity == "ERROR") { + else if (verbosity == "ERROR") { SetMinLogLevel(ERROR); } else if (verbosity == "WARNING") { SetMinLogLevel(WARNING); } - else if (verbosity == "GPR_DEBUG") { + else if (verbosity == "DEBUG") { SetGlobalVLogLevel(2); SetMinLogLevel(INFO); } From 8027216390aa4ac1725fc477ca4d1bf7ba2d98a3 Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Mon, 13 May 2024 10:11:37 +0530 Subject: [PATCH 23/26] Update L117-core-replace-gpr-logging-with-abseil-logging.md --- L117-core-replace-gpr-logging-with-abseil-logging.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/L117-core-replace-gpr-logging-with-abseil-logging.md b/L117-core-replace-gpr-logging-with-abseil-logging.md index 3276eab03..0cebd4286 100644 --- a/L117-core-replace-gpr-logging-with-abseil-logging.md +++ b/L117-core-replace-gpr-logging-with-abseil-logging.md @@ -76,13 +76,13 @@ void SomeInitFunctionCalledByGrpcInit() { } void gpr_set_log_verbosity(gpr_log_severity verbosity) { - if (verbosity == GPR_INFO) { + if (verbosity == GPR_LOG_SEVERITY_INFO) { SetMinLogLevel(INFO); } - else if (verbosity == GPR_ERROR) { + else if (verbosity == GPR_LOG_SEVERITY_ERROR) { SetMinLogLevel(ERROR); } - else if (verbosity == GPR_DEBUG) { + else if (verbosity == GPR_LOG_SEVERITY_DEBUG) { SetGlobalVLogLevel(2); SetMinLogLevel(INFO); } From bb0db9f2d3806eba9e8f7781d387cd3c0e7bd40c Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Thu, 16 May 2024 11:13:05 +0530 Subject: [PATCH 24/26] Update L117-core-replace-gpr-logging-with-abseil-logging.md --- ...replace-gpr-logging-with-abseil-logging.md | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/L117-core-replace-gpr-logging-with-abseil-logging.md b/L117-core-replace-gpr-logging-with-abseil-logging.md index 0cebd4286..13caa6846 100644 --- a/L117-core-replace-gpr-logging-with-abseil-logging.md +++ b/L117-core-replace-gpr-logging-with-abseil-logging.md @@ -50,7 +50,7 @@ We are proposing to remove all instances of gpr logging and asserts and replace * `gpr_should_log` - This wont be needed anymore. ### Will work similar to before -* `gpr_set_log_verbosity` and GRPC_VERBOSITY will work as follows +* `GRPC_VERBOSITY` will work as follows ``` void SomeInitFunctionCalledByGrpcInit() { @@ -75,23 +75,6 @@ void SomeInitFunctionCalledByGrpcInit() { } } -void gpr_set_log_verbosity(gpr_log_severity verbosity) { - if (verbosity == GPR_LOG_SEVERITY_INFO) { - SetMinLogLevel(INFO); - } - else if (verbosity == GPR_LOG_SEVERITY_ERROR) { - SetMinLogLevel(ERROR); - } - else if (verbosity == GPR_LOG_SEVERITY_DEBUG) { - SetGlobalVLogLevel(2); - SetMinLogLevel(INFO); - } - else { - LOG(ERROR) << "Unknown log verbosity: " << verbosity; - } -} -``` - ## Rationale ### Advantages From 5bf02233a88f87634e004d3d5edd315aaed6cab1 Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Thu, 16 May 2024 12:40:12 +0530 Subject: [PATCH 25/26] Update L117-core-replace-gpr-logging-with-abseil-logging.md --- ...replace-gpr-logging-with-abseil-logging.md | 37 +++++++++---------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/L117-core-replace-gpr-logging-with-abseil-logging.md b/L117-core-replace-gpr-logging-with-abseil-logging.md index 13caa6846..0f1864e48 100644 --- a/L117-core-replace-gpr-logging-with-abseil-logging.md +++ b/L117-core-replace-gpr-logging-with-abseil-logging.md @@ -54,25 +54,24 @@ We are proposing to remove all instances of gpr logging and asserts and replace ``` void SomeInitFunctionCalledByGrpcInit() { - optional verbosity = GetEnv("GRPC_VERBOSITY"); - if (verbosity.has_value()) { - if (verbosity == "INFO") { - SetMinLogLevel(INFO); - } - else if (verbosity == "ERROR") { - SetMinLogLevel(ERROR); - } - else if (verbosity == "WARNING") { - SetMinLogLevel(WARNING); - } - else if (verbosity == "DEBUG") { - SetGlobalVLogLevel(2); - SetMinLogLevel(INFO); - } - else { - LOG(ERROR) << "Unknown log verbosity: " << verbosity; - } - } + absl::optional verbosity = grpc_core::GetEnv("GRPC_VERBOSITY"); + if (verbosity.has_value()) { + VLOG(2) << "Log verbosity: " << verbosity.value(); + if (absl::EqualsIgnoreCase(verbosity.value(), "INFO")) { + absl::SetMinLogLevel(absl::LogSeverityAtLeast::kInfo); + } else if (absl::EqualsIgnoreCase(verbosity.value(), "ERROR")) { + absl::SetMinLogLevel(absl::LogSeverityAtLeast::kError); + } else if (absl::EqualsIgnoreCase(verbosity.value(), "WARNING")) { + absl::SetMinLogLevel(absl::LogSeverityAtLeast::kWarning); + } else if (absl::EqualsIgnoreCase(verbosity.value(), "DEBUG")) { + absl::SetGlobalVLogLevel(2); + absl::SetMinLogLevel(absl::LogSeverityAtLeast::kInfo); + } else { + LOG(ERROR) << "Unknown log verbosity: " << verbosity.value(); + } + } else { + VLOG(2) << "No verbosity set. Default will be used."; + } } ## Rationale From 4198fc16028146e60688962363e27d274c3c0f4c Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Thu, 16 May 2024 12:42:49 +0530 Subject: [PATCH 26/26] Update L117-core-replace-gpr-logging-with-abseil-logging.md --- L117-core-replace-gpr-logging-with-abseil-logging.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/L117-core-replace-gpr-logging-with-abseil-logging.md b/L117-core-replace-gpr-logging-with-abseil-logging.md index 0f1864e48..c64dcf0ce 100644 --- a/L117-core-replace-gpr-logging-with-abseil-logging.md +++ b/L117-core-replace-gpr-logging-with-abseil-logging.md @@ -44,6 +44,7 @@ We are proposing to remove all instances of gpr logging and asserts and replace * `GPR_DEBUG_ASSERT(...)` with `DCHECK(...)` * `gpr_assertion_failed` with `CHECK(...)` * `gpr_set_log_function` - This function will be removed. Teams can consider usage of [LogSink](https://github.com/abseil/abseil-cpp/blob/fa57bfc573453d57a38552eedcce894b0e2d9f5e/absl/log/log_sink.h) and [AddLogSink](https://github.com/abseil/abseil-cpp/blob/fa57bfc573453d57a38552eedcce894b0e2d9f5e/absl/log/log_sink_registry.h). +* `gpr_set_log_verbosity` will be removed. ### Functions that will be removed * `gpr_log_severity_string` - This wont be needed anymore. @@ -73,6 +74,7 @@ void SomeInitFunctionCalledByGrpcInit() { VLOG(2) << "No verbosity set. Default will be used."; } } +``` ## Rationale