Navigation Menu

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

Apply tapping logic only when taps are active #142

Merged
merged 14 commits into from Nov 30, 2018
Merged

Conversation

olix0r
Copy link
Member

@olix0r olix0r commented Nov 27, 2018

Previously, as the proxy processed requests, it would:

  1. Obtain the taps mutex ~4x per request to determine whether taps are active.
  2. Construct an "event" ~4x per request, regardless of whether any taps were
    active.

Furthermore, this relied on fragile caching logic, where the grpc server
manages individual stream states in a Map to determine when all streams have
been completed. And, beyond the complexity of caching, this approach makes it
difficult to expand Tap functionality (for instance, to support tapping of
payloads).

This change entirely rewrites the proxy's Tap logic to (1) prevent the need
to acquire muteces in the request path, (2) only produce events as needed to
satisfy tap requests, and (3) provide clear (private) API boundaries between
the Tap server and Stack, completely hiding gRPC details from the tap service.

The tap::service module now provides a middleware that is generic over a
way to discover Taps; and the tap::grpc module (previously,
control::observe), implements a gRPC service that advertises Taps such that
their lifetimes are managed properly, leveraging RAII instead of hand-rolled
map-based caching.

There is one user-facing change: tap stream IDs are now calculated relative to
the tap server. The base id is assigned from the count of tap requests that have
been made to the proxy; and the stream ID corresponds to an integer on [0, limit).

@olix0r olix0r self-assigned this Nov 27, 2018
@olix0r
Copy link
Member Author

olix0r commented Nov 27, 2018

This review is probably less-useful as a diff. I recommend checking out the branch and reading through the tap module.

I'm especially looking for aspects of this that need more explanation, so please ask questions!.

Copy link
Contributor

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

This a huge improvement over the mess we made in control::*, \o/

src/tap/service.rs Outdated Show resolved Hide resolved
@olix0r olix0r merged commit 82524e4 into master Nov 30, 2018
@olix0r olix0r deleted the ver/tap-register branch November 30, 2018 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants