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.

Sign up for GitHub

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

Jump to bottom

L117: C-core: replace gpr logging with absl logging #425

Merged
merged 26 commits into from
May 17, 2024
Merged

L117: C-core: replace gpr logging with absl logging #425

merged 26 commits into from
May 17, 2024

Conversation

tanvi-jagtap
Copy link
Contributor

No description provided.

@tanvi-jagtap tanvi-jagtap requested a review from ctiller April 17, 2024 16:30
@eugeneo
Copy link
Contributor

eugeneo commented Apr 17, 2024

Questions:

  1. Will gpr_log and friends get removed or will they remain and wrap ABSL APIs?
  2. Are there any ABSL logging features we should not use in gRPC?
  3. Will GRPC_VERBOSITY and GRPC_TRACE env variables keep working?
  4. Is there a way to suppress the absl::InitializeLog warning for our users? It will force them to add dependency to ABSL to their main function and I anticipate a lot of grumbling.

@tanvi-jagtap
Copy link
Contributor Author

Questions:

  1. Will gpr_log and friends get removed or will they remain and wrap ABSL APIs?

Yes. Removing them is the plan.

  1. Are there any ABSL logging features we should not use in gRPC?

Not that I know of. However @ctiller may be able to validate that.

  1. Will GRPC_VERBOSITY and GRPC_TRACE env variables keep working?

No changes are expected to GRPC_TRACE and related functions.
I will get back to you on GRPC_VERBOSITY . There are some things I need to look into.

  1. Is there a way to suppress the absl::InitializeLog warning for our users? It will force them to add dependency to ABSL to their main function and I anticipate a lot of grumbling.

I will get back on this too.

@ctiller
Copy link
Member

ctiller commented Apr 17, 2024

Re: suppressing the absl::InitializeLog warning: I highly recommend not doing so. That behavior is controlled by absl, and if we introduce mechanisms over top by default then we're fighting their defaults which is going to lead to more confusion - especially since it's unallowed to call InitializeLog twice.

Ack that there'll be some grumbling.

Re: GRPC_VERBOSITY: I'd propose we introduce some code that if it's set overrides the absl defaults -- but if it's not set we exert no opinion on what absl does.

@eugeneo
Copy link
Contributor

eugeneo commented Apr 17, 2024

Ack that there'll be some grumbling.

This may cause a lot of frustration with our users, i.e. people may perceive it as some real issue and end up spending time trying to figure out how to fix it, then having to add a dependency to ABSL log (and that may be challenging for people with weird build systems).

I feel like we need to be user friendly.

@ctiller ctiller changed the title gRFC for gpr logging to absl logging L116: gRFC for gpr logging to absl logging Apr 23, 2024
@tanvi-jagtap tanvi-jagtap requested a review from markdroth April 25, 2024 04:08
@tanvi-jagtap tanvi-jagtap changed the title L116: gRFC for gpr logging to absl logging L117: gRFC for gpr logging to absl logging Apr 29, 2024
@markdroth markdroth changed the title L117: gRFC for gpr logging to absl logging L117: C-core: replace gpr logging with absl logging May 1, 2024
markdroth
markdroth reviewed May 1, 2024
View reviewed changes
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this up! Comments are mostly costmetic.

Please let me know if you have any questions. Thanks!

L117-cpp-replacing-gpr-logging-with-abseil-logging.md Outdated Show resolved Hide resolved
L117-cpp-replacing-gpr-logging-with-abseil-logging.md Outdated Show resolved Hide resolved
L117-cpp-replacing-gpr-logging-with-abseil-logging.md Outdated Show resolved Hide resolved
L117-cpp-replacing-gpr-logging-with-abseil-logging.md Outdated Show resolved Hide resolved
L117-cpp-replacing-gpr-logging-with-abseil-logging.md Outdated Show resolved Hide resolved
L117-cpp-replacing-gpr-logging-with-abseil-logging.md Outdated Show resolved Hide resolved
L117-cpp-replacing-gpr-logging-with-abseil-logging.md Outdated Show resolved Hide resolved
L117-cpp-replacing-gpr-logging-with-abseil-logging.md Outdated Show resolved Hide resolved
markdroth
markdroth approved these changes May 10, 2024
View reviewed changes
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

L117-core-replace-gpr-logging-with-abseil-logging.md Outdated Show resolved Hide resolved
ctiller
ctiller approved these changes May 14, 2024
View reviewed changes
L116-replacing-gpr-logging-with-abseil-logging.md Outdated Show resolved Hide resolved
L117-core-replace-gpr-logging-with-abseil-logging.md Outdated Show resolved Hide resolved
@tanvi-jagtap tanvi-jagtap merged commit a2aaecc into grpc:master May 17, 2024
1 check passed
@tanvi-jagtap tanvi-jagtap deleted the tjagtap_gpr branch May 17, 2024 04:03
This was referenced Jun 7, 2024
Closed
Closed
copybara-service bot pushed a commit to grpc/grpc that referenced this pull request Jun 7, 2024
@tanvi-jagtap @copybara-github
[grpc][Gpr_To_Absl_Logging] Deleting multiple instances of gpr_set_lo…
1dbfd4c 
…g_function (#36833)

Deleting multiple instances of gpr_set_log_function .
This function will be deleted soon.
grpc/proposal#425

Closes #36833

COPYBARA_INTEGRATE_REVIEW=#36833 from tanvi-jagtap:remove_gpr_log_partial_code 17517ef
PiperOrigin-RevId: 641268299
copybara-service bot pushed a commit to grpc/grpc that referenced this pull request Jun 8, 2024
@tanvi-jagtap @copybara-github
[Gpr_To_Absl_Logging] Migrating from gpr to absl logging - gpr_set_lo…
cd595a8 
…g_function (#36844)

[Gpr_To_Absl_Logging] Migrating from gpr to absl logging - gpr_set_log_function

Adding a warning for users of gpr_set_log_function.

This function will be deleted soon.
grpc/proposal#425

Other options considered ( and discarded )
1. Using DFATAL instead of ERROR. Discarded this because we still have one critical unit test to clean up before we delete gpr_set_log_function. This test will fail if we use DFATAL. EXPECT_DFATAL macro is not available to be used.
2. Considered making a new absl log sink , and then that log sink should have a function pointer with the same signature as gpr_log_func. This function pointer can be set from gpr_set_log_function. However if this custom function uses absl from within it, that will cause infinite recursion. Made me drop the idea.

Closes #36844

COPYBARA_INTEGRATE_REVIEW=#36844 from tanvi-jagtap:assert_gpr_set_log_function 805de35
PiperOrigin-RevId: 641473969
This was referenced Jun 18, 2024
Closed
Closed
copybara-service bot pushed a commit to grpc/grpc that referenced this pull request Jun 20, 2024
@tanvi-jagtap @copybara-github
[Gpr_To_Absl_Logging] Fixing bugs (#36961)
4c9db80 
### Problem 1

Context :
gpr_log() internally calls gpr_log_message() .
gpr_log_message() may either call gpr_default_log() or a user provided logging function.
gpr_default_log() uses absl LOG. This is the default. Most users will log this way.
For the small percentage of users who have customized the logging function, gpr_log will log to custom this function.

Problem :
We have converted half the instances of gpr_log to absl LOG().
For users who use the defaults - there will be no issue.
For the users who use a customized logging function
1. All the absl LOGs will log to the absl log sink.
2. All the gpr_log statements will log via this user provided function.
This is in-consistent behaviour and will cause confusion and difficulty in debugging.

Solution:
All logs should go to the same sink.
So we decided to make gpr_set_log_function a no op in this release.
The function will be deleted in the next release.
grpc/proposal#425

### Problem 2

Context :
gpr_should_log is used to avoid computing expensive stuff for logging if the log is not going to be visible.

Problem :
gpr_should_log was referencing the GRPC_VERBOSITY flag and values set by gpr_set_log_verbosity .
However, actual logging happens based on the absl settings.
This is incorrect. Because if the old settings are not honoured, they should not be checked and no decision in code should be made based on settings which are not going to get used.

Solution :
Given the above changes in Problem 1, since all custom logging is disabled, all logging from gRPC with honour the absl LOG settings. Hence we modified the gpr_should_log function to refer to absl settings.

### Problem 3

We still have the issue of php using a custom log sink. We will address this in a separate PR.

### Problem 4

Tests related to test/core/end2end/tests/no_logging.cc are broken . These will be fixed in another PR.

Closes #36961

COPYBARA_INTEGRATE_REVIEW=#36961 from tanvi-jagtap:fix_gpr_should_log 70c3224
PiperOrigin-RevId: 645096418
tanvi-jagtap added a commit to tanvi-jagtap/tjagtap_grpc that referenced this pull request Jun 20, 2024
@tanvi-jagtap
[Gpr_To_Absl_Logging] Fixing bugs (grpc#36961)
7b19d58 
### Problem 1

Context :
gpr_log() internally calls gpr_log_message() .
gpr_log_message() may either call gpr_default_log() or a user provided logging function.
gpr_default_log() uses absl LOG. This is the default. Most users will log this way.
For the small percentage of users who have customized the logging function, gpr_log will log to custom this function.

Problem :
We have converted half the instances of gpr_log to absl LOG().
For users who use the defaults - there will be no issue.
For the users who use a customized logging function
1. All the absl LOGs will log to the absl log sink.
2. All the gpr_log statements will log via this user provided function.
This is in-consistent behaviour and will cause confusion and difficulty in debugging.

Solution:
All logs should go to the same sink.
So we decided to make gpr_set_log_function a no op in this release.
The function will be deleted in the next release.
grpc/proposal#425

### Problem 2

Context :
gpr_should_log is used to avoid computing expensive stuff for logging if the log is not going to be visible.

Problem :
gpr_should_log was referencing the GRPC_VERBOSITY flag and values set by gpr_set_log_verbosity .
However, actual logging happens based on the absl settings.
This is incorrect. Because if the old settings are not honoured, they should not be checked and no decision in code should be made based on settings which are not going to get used.

Solution :
Given the above changes in Problem 1, since all custom logging is disabled, all logging from gRPC with honour the absl LOG settings. Hence we modified the gpr_should_log function to refer to absl settings.

### Problem 3

We still have the issue of php using a custom log sink. We will address this in a separate PR.

### Problem 4

Tests related to test/core/end2end/tests/no_logging.cc are broken . These will be fixed in another PR.

Closes grpc#36961

COPYBARA_INTEGRATE_REVIEW=grpc#36961 from tanvi-jagtap:fix_gpr_should_log 70c3224
PiperOrigin-RevId: 645096418
This was referenced Jun 20, 2024
Closed
Merged
yashykt pushed a commit to grpc/grpc that referenced this pull request Jun 20, 2024
@tanvi-jagtap
[Gpr_To_Absl_Logging] Fixing bugs (#36961) (#36996)
44fc8c0 
Backport #36961 to v1.65.x

### Problem 1

Context :
gpr_log() internally calls gpr_log_message() .
gpr_log_message() may either call gpr_default_log() or a user provided
logging function. gpr_default_log() uses absl LOG. This is the default.
Most users will log this way. For the small percentage of users who have
customized the logging function, gpr_log will log to custom this
function.

Problem :
We have converted half the instances of gpr_log to absl LOG(). For users
who use the defaults - there will be no issue. For the users who use a
customized logging function
1. All the absl LOGs will log to the absl log sink.
2. All the gpr_log statements will log via this user provided function.
This is in-consistent behaviour and will cause confusion and difficulty
in debugging.

Solution:
All logs should go to the same sink.
So we decided to make gpr_set_log_function a no op in this release. The
function will be deleted in the next release.
grpc/proposal#425

### Problem 2

Context :
gpr_should_log is used to avoid computing expensive stuff for logging if
the log is not going to be visible.

Problem :
gpr_should_log was referencing the GRPC_VERBOSITY flag and values set by
gpr_set_log_verbosity . However, actual logging happens based on the
absl settings. This is incorrect. Because if the old settings are not
honoured, they should not be checked and no decision in code should be
made based on settings which are not going to get used.

Solution :
Given the above changes in Problem 1, since all custom logging is
disabled, all logging from gRPC with honour the absl LOG settings. Hence
we modified the gpr_should_log function to refer to absl settings.

### Problem 3

We still have the issue of php using a custom log sink. We will address
this in a separate PR.

### Problem 4

Tests related to test/core/end2end/tests/no_logging.cc are broken .
These will be fixed in another PR.

Closes #36961

COPYBARA_INTEGRATE_REVIEW=#36961 from
tanvi-jagtap:fix_gpr_should_log 70c3224
PiperOrigin-RevId: 645096418
sourabhsinghs pushed a commit to sourabhsinghs/grpc that referenced this pull request Sep 26, 2024
@tanvi-jagtap @sourabhsinghs
[grpc][Gpr_To_Absl_Logging] Deleting multiple instances of gpr_set_lo…
274a854 
…g_function (grpc#36833)

Deleting multiple instances of gpr_set_log_function .
This function will be deleted soon.
grpc/proposal#425

Closes grpc#36833

COPYBARA_INTEGRATE_REVIEW=grpc#36833 from tanvi-jagtap:remove_gpr_log_partial_code 17517ef
PiperOrigin-RevId: 641268299
sourabhsinghs pushed a commit to sourabhsinghs/grpc that referenced this pull request Sep 26, 2024
@tanvi-jagtap @sourabhsinghs
[Gpr_To_Absl_Logging] Migrating from gpr to absl logging - gpr_set_lo…
e90ab3c 
…g_function (grpc#36844)

[Gpr_To_Absl_Logging] Migrating from gpr to absl logging - gpr_set_log_function

Adding a warning for users of gpr_set_log_function.

This function will be deleted soon.
grpc/proposal#425

Other options considered ( and discarded )
1. Using DFATAL instead of ERROR. Discarded this because we still have one critical unit test to clean up before we delete gpr_set_log_function. This test will fail if we use DFATAL. EXPECT_DFATAL macro is not available to be used.
2. Considered making a new absl log sink , and then that log sink should have a function pointer with the same signature as gpr_log_func. This function pointer can be set from gpr_set_log_function. However if this custom function uses absl from within it, that will cause infinite recursion. Made me drop the idea.

Closes grpc#36844

COPYBARA_INTEGRATE_REVIEW=grpc#36844 from tanvi-jagtap:assert_gpr_set_log_function 805de35
PiperOrigin-RevId: 641473969
sourabhsinghs pushed a commit to sourabhsinghs/grpc that referenced this pull request Sep 26, 2024
@tanvi-jagtap @sourabhsinghs
[Gpr_To_Absl_Logging] Fixing bugs (grpc#36961)
a5e84e9 
### Problem 1

Context :
gpr_log() internally calls gpr_log_message() .
gpr_log_message() may either call gpr_default_log() or a user provided logging function.
gpr_default_log() uses absl LOG. This is the default. Most users will log this way.
For the small percentage of users who have customized the logging function, gpr_log will log to custom this function.

Problem :
We have converted half the instances of gpr_log to absl LOG().
For users who use the defaults - there will be no issue.
For the users who use a customized logging function
1. All the absl LOGs will log to the absl log sink.
2. All the gpr_log statements will log via this user provided function.
This is in-consistent behaviour and will cause confusion and difficulty in debugging.

Solution:
All logs should go to the same sink.
So we decided to make gpr_set_log_function a no op in this release.
The function will be deleted in the next release.
grpc/proposal#425

### Problem 2

Context :
gpr_should_log is used to avoid computing expensive stuff for logging if the log is not going to be visible.

Problem :
gpr_should_log was referencing the GRPC_VERBOSITY flag and values set by gpr_set_log_verbosity .
However, actual logging happens based on the absl settings.
This is incorrect. Because if the old settings are not honoured, they should not be checked and no decision in code should be made based on settings which are not going to get used.

Solution :
Given the above changes in Problem 1, since all custom logging is disabled, all logging from gRPC with honour the absl LOG settings. Hence we modified the gpr_should_log function to refer to absl settings.

### Problem 3

We still have the issue of php using a custom log sink. We will address this in a separate PR.

### Problem 4

Tests related to test/core/end2end/tests/no_logging.cc are broken . These will be fixed in another PR.

Closes grpc#36961

COPYBARA_INTEGRATE_REVIEW=grpc#36961 from tanvi-jagtap:fix_gpr_should_log 70c3224
PiperOrigin-RevId: 645096418
@meztez meztez mentioned this pull request Oct 18, 2024
Closed
paulosjca pushed a commit to paulosjca/grpc that referenced this pull request Nov 25, 2024
@tanvi-jagtap @paulosjca
[grpc][Gpr_To_Absl_Logging] Deleting multiple instances of gpr_set_lo…
e1b6450 
…g_function (grpc#36833)

Deleting multiple instances of gpr_set_log_function .
This function will be deleted soon.
grpc/proposal#425

Closes grpc#36833

COPYBARA_INTEGRATE_REVIEW=grpc#36833 from tanvi-jagtap:remove_gpr_log_partial_code 17517ef
PiperOrigin-RevId: 641268299
paulosjca pushed a commit to paulosjca/grpc that referenced this pull request Nov 25, 2024
@tanvi-jagtap @paulosjca
[Gpr_To_Absl_Logging] Migrating from gpr to absl logging - gpr_set_lo…
45d2279 
…g_function (grpc#36844)

[Gpr_To_Absl_Logging] Migrating from gpr to absl logging - gpr_set_log_function

Adding a warning for users of gpr_set_log_function.

This function will be deleted soon.
grpc/proposal#425

Other options considered ( and discarded )
1. Using DFATAL instead of ERROR. Discarded this because we still have one critical unit test to clean up before we delete gpr_set_log_function. This test will fail if we use DFATAL. EXPECT_DFATAL macro is not available to be used.
2. Considered making a new absl log sink , and then that log sink should have a function pointer with the same signature as gpr_log_func. This function pointer can be set from gpr_set_log_function. However if this custom function uses absl from within it, that will cause infinite recursion. Made me drop the idea.

Closes grpc#36844

COPYBARA_INTEGRATE_REVIEW=grpc#36844 from tanvi-jagtap:assert_gpr_set_log_function 805de35
PiperOrigin-RevId: 641473969
paulosjca pushed a commit to paulosjca/grpc that referenced this pull request Nov 25, 2024
@tanvi-jagtap @paulosjca
[Gpr_To_Absl_Logging] Fixing bugs (grpc#36961)
40bbd66 
### Problem 1

Context :
gpr_log() internally calls gpr_log_message() .
gpr_log_message() may either call gpr_default_log() or a user provided logging function.
gpr_default_log() uses absl LOG. This is the default. Most users will log this way.
For the small percentage of users who have customized the logging function, gpr_log will log to custom this function.

Problem :
We have converted half the instances of gpr_log to absl LOG().
For users who use the defaults - there will be no issue.
For the users who use a customized logging function
1. All the absl LOGs will log to the absl log sink.
2. All the gpr_log statements will log via this user provided function.
This is in-consistent behaviour and will cause confusion and difficulty in debugging.

Solution:
All logs should go to the same sink.
So we decided to make gpr_set_log_function a no op in this release.
The function will be deleted in the next release.
grpc/proposal#425

### Problem 2

Context :
gpr_should_log is used to avoid computing expensive stuff for logging if the log is not going to be visible.

Problem :
gpr_should_log was referencing the GRPC_VERBOSITY flag and values set by gpr_set_log_verbosity .
However, actual logging happens based on the absl settings.
This is incorrect. Because if the old settings are not honoured, they should not be checked and no decision in code should be made based on settings which are not going to get used.

Solution :
Given the above changes in Problem 1, since all custom logging is disabled, all logging from gRPC with honour the absl LOG settings. Hence we modified the gpr_should_log function to refer to absl settings.

### Problem 3

We still have the issue of php using a custom log sink. We will address this in a separate PR.

### Problem 4

Tests related to test/core/end2end/tests/no_logging.cc are broken . These will be fixed in another PR.

Closes grpc#36961

COPYBARA_INTEGRATE_REVIEW=grpc#36961 from tanvi-jagtap:fix_gpr_should_log 70c3224
PiperOrigin-RevId: 645096418
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markdroth markdroth markdroth approved these changes

@ctiller ctiller ctiller approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tanvi-jagtap @eugeneo @ctiller @markdroth