Skip to content

[EventEngine] Add more granular trace flags#32376

Merged
drfloob merged 1 commit intogrpc:masterfrom
drfloob:better-ee-trace-flags
Feb 14, 2023
Merged

[EventEngine] Add more granular trace flags#32376
drfloob merged 1 commit intogrpc:masterfrom
drfloob:better-ee-trace-flags

Conversation

@drfloob
Copy link
Copy Markdown
Member

@drfloob drfloob commented Feb 14, 2023

The set of trace flags is now:

  • event_engine
  • event_engine_endpoint
  • event_engine_endpoint_data: additionally log all sent/received data, similar to what the shims do.
  • event_engine_poller

@drfloob drfloob requested a review from Vignesh2208 February 14, 2023 19:01
@drfloob drfloob added the release notes: no Indicates if PR should not be in release notes label Feb 14, 2023
@ctiller
Copy link
Copy Markdown
Member

ctiller commented Feb 14, 2023

Not for this PR, but to put it out there:
What do we feel about hierarchical trace flags?

GRPC_TRACE=event_engine/endpoint
GRPC_TRACE=event_engine/{endpoint,poller}
GRPC_TRACE=event_engine/*

@drfloob
Copy link
Copy Markdown
Member Author

drfloob commented Feb 14, 2023

What do we feel about hierarchical trace flags?

I'd use it here for sure. I think there are other small (<= 3 sub-traces) potential uses with call, client_channel, subchannel. XDS also has quite a few, but I'm not sure how valuable it would be to enable the bulk of them at once.

Regarding the change, the trace flags are sort of public API, right? Changing existing flags could upset some folks who rely on them already. If we did any flag restructuring, we may want to ensure the old flags are still supported.

@Vignesh2208
Copy link
Copy Markdown
Contributor

Thanks for creating this AJ. LGTM

@drfloob drfloob merged commit dd07fd8 into grpc:master Feb 14, 2023
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Feb 14, 2023
drfloob added a commit that referenced this pull request Feb 15, 2023
A handful of problems were identified while writing the
WindowsEventEngine Listener. To make the listener review easier, these
fixes can be landed separately.

This is built upon #32376

Problems that are fixed in this PR:
* `OnConnectCompleted` held a Mutex while calling the user callback,
which can deadlock.
* The WinSocket and some associated data needs to remain alive after the
Endpoint destroyed, since Windows IOCP still needs to use some of that
data. Endpoint destruction and socket shutdown are now decoupled, with
the socket managed by a shared_ptr.

<!--

If you know who should review your pull request, please assign it to
that
person, otherwise the pull request would get assigned randomly.

If your pull request is for a specific language, please add the
appropriate
lang label.

-->

---------

Co-authored-by: drfloob <drfloob@users.noreply.github.com>
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
The set of trace flags is now:
* event_engine
* event_engine_endpoint
* event_engine_endpoint_data: additionally log all sent/received data,
similar to what the shims do.
* event_engine_poller

<!--

If you know who should review your pull request, please assign it to
that
person, otherwise the pull request would get assigned randomly.

If your pull request is for a specific language, please add the
appropriate
lang label.

-->
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
A handful of problems were identified while writing the
WindowsEventEngine Listener. To make the listener review easier, these
fixes can be landed separately.

This is built upon grpc#32376

Problems that are fixed in this PR:
* `OnConnectCompleted` held a Mutex while calling the user callback,
which can deadlock.
* The WinSocket and some associated data needs to remain alive after the
Endpoint destroyed, since Windows IOCP still needs to use some of that
data. Endpoint destruction and socket shutdown are now decoupled, with
the socket managed by a shared_ptr.

<!--

If you know who should review your pull request, please assign it to
that
person, otherwise the pull request would get assigned randomly.

If your pull request is for a specific language, please add the
appropriate
lang label.

-->

---------

Co-authored-by: drfloob <drfloob@users.noreply.github.com>
wanlin31 pushed a commit that referenced this pull request May 18, 2023
The set of trace flags is now:
* event_engine
* event_engine_endpoint
* event_engine_endpoint_data: additionally log all sent/received data,
similar to what the shims do.
* event_engine_poller

<!--

If you know who should review your pull request, please assign it to
that
person, otherwise the pull request would get assigned randomly.

If your pull request is for a specific language, please add the
appropriate
lang label.

-->
wanlin31 pushed a commit that referenced this pull request May 18, 2023
A handful of problems were identified while writing the
WindowsEventEngine Listener. To make the listener review easier, these
fixes can be landed separately.

This is built upon #32376

Problems that are fixed in this PR:
* `OnConnectCompleted` held a Mutex while calling the user callback,
which can deadlock.
* The WinSocket and some associated data needs to remain alive after the
Endpoint destroyed, since Windows IOCP still needs to use some of that
data. Endpoint destruction and socket shutdown are now decoupled, with
the socket managed by a shared_ptr.

<!--

If you know who should review your pull request, please assign it to
that
person, otherwise the pull request would get assigned randomly.

If your pull request is for a specific language, please add the
appropriate
lang label.

-->

---------

Co-authored-by: drfloob <drfloob@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

imported Specifies if the PR has been imported to the internal repository lang/core per-call-memory/neutral per-channel-memory/neutral release notes: no Indicates if PR should not be in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants