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.

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

[logging] Centralize configuration for trace flags #36576

Open
wants to merge 75 commits into
base: master
Choose a base branch
from

Conversation

drfloob
Copy link
Member

@drfloob drfloob commented May 10, 2024

All TraceFlags are now configured in src/core/lib/debug/trace_flags.yaml. The format is:

my_flag:
  default: false                   # the default value; default=false
  description: Some Description
  debug_only: false                # debug_only flags only work in debug builds; default=false
  internal: false                  # internal flags will not show up in documentation; default=false

To regenerate the trace flag source code, run tools/codegen/core/gen_trace_flags.py (requires mako). This script is also run when sanity checking.

This PR also adds two new features:

Glob-based flag configuration

Trace flag configuration now supports ? (single wildcard character) and * (one or more wildcard characters). For example, using GRPC_TRACE='event_engine*' will enable all flags that match that glob. It expands to:

  • event_engine
  • event_engine_client_channel_resolver
  • event_engine_dns
  • event_engine_endpoint
  • event_engine_endpoint_data
  • event_engine_poller

A cleaner trace-logging macro in abseil logging format

If your goal is only to add log statements when the fault_injection_filter trace flag is enabled, you can use the macro:

GRPC_TRACE_LOG(fault_injection, INFO) << "Filtered:" << 42;

When the trace flag is enabled, the the log will show something like this:

I0000 00:00:1715733657.430042      16 file.cc:174] Filtered:42

Note: just like with the gpr_log to abseil logging conversion, the pre-existing trace logging usages can be replaced with the new tracing macro across multiple PRs.

@drfloob drfloob requested review from ctiller and markdroth and removed request for markdroth May 10, 2024 06:56
Copy link
Member Author

@drfloob drfloob left a comment

Choose a reason for hiding this comment

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

Pretty close now!

src/core/lib/debug/trace_flags.yaml Outdated Show resolved Hide resolved
src/core/lib/resource_quota/trace.cc Outdated Show resolved Hide resolved
src/core/lib/slice/slice_refcount.cc Outdated Show resolved Hide resolved
test/core/test_util/BUILD Outdated Show resolved Hide resolved
@drfloob drfloob requested a review from markdroth May 16, 2024 17:10
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.

This looks fantastic!

test/core/handshake/client_ssl.cc Outdated Show resolved Hide resolved
@tanvi-jagtap
Copy link
Contributor

@drfloob : Please can you let me know approximately when this PR will be submitted. I will avoid editing the files in the PR for some time to avoid more merge conflicts for both of us.
In the meantime I can finish all other files that need gpr to absl migration.

LOG(INFO) << absl::StrFormat("%20.20s - %30.30s - %5.10s", msg,
SSL_state_string_long(ssl),
SSL_state_string(ssl));
if (where & flag) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer (where & flag) != 0 here, for the same reasons we usually write != nullptr for pointers -- this is an int not a bool.

Copy link
Member

Choose a reason for hiding this comment

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

+1 -- but don't need the parens.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. It does require parens though, != has higher precedence than &. http://go/cppref/cpp/language/operator_precedence

@drfloob
Copy link
Member Author

drfloob commented May 21, 2024

@drfloob : Please can you let me know approximately when this PR will be submitted. I will avoid editing the files in the PR for some time to avoid more merge conflicts for both of us. In the meantime I can finish all other files that need gpr to absl migration.

@tanvi-jagtap Thanks. I'll try to wrap it up this week, I have a bit of codegen work to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants