Conversation
|
@alyssawilk, this came out of a discussion our team had about being able to correlate stream behavior grouped by underlying upstream connection. I wanted to put a draft up to discuss were and when to get this data. It looks to me from the existing behavior in the code here: d5b6e79#diff-b61a05a18225decb27f5620aefda1646c259e314547a7d985c369f62a684c12bR420 that the upstream connection populates its "downstream" side because the codec client (and the underlying connection) could be either downstream or upstream. So my first question to your is if this PR should support both upstream and downstream connection IDs, and fill only downstream when only a single direction makes sense. Ultimately all I want is for the stream info to have access to the upstream connection id, and this seemed like the cleanest way to do so. However, I am not tied to this being the best way to obtain this information. cc @goaway |
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Yeah, this sounds reasonable to me.
@mattklein123 @RyanTheOptimist for any other thoughts
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
|
@alyssawilk updated! |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks LGTM at a high level. Will defer to @alyssawilk for further review.
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
LGTM modulo one comment request :-)
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
|
@alyssawilk if you could re-stamp that would be great. I realized that there was more involved in cleaning up the connectionID function that used to exist. I'll clean it up in a subsequent PR, and rolled back the initial deletion. |
|
@alyssawilk @soulxu I have updated with @soulxu's recommendation. We should be good now |
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
|
thanks for update! LGTM |
|
/retest |
|
Retrying Azure Pipelines: |
…bridge-stream * upstream/main: stream info: add upstream connection id (envoyproxy#17512) runtime: removing require_ocsp_response_for_must_staple_certs (envoyproxy#17541) tooling: Fixes/cleanups/tests for `rst_checks.py` (envoyproxy#17589) Add a new HappyEyeballsConnectionImpl class (envoyproxy#17371) hcm/listener_manager: remove deprecated v2 config handling. (envoyproxy#17586) tools: simplify bazel deps query (envoyproxy#17599) tooling: Move tempdir/cleanup to base runner class (envoyproxy#17590) tools: fix missing format strings (envoyproxy#17600) tooling: Add `@envoy` prefix in `envoy_py_` macros (envoyproxy#17588) stream info: add attempt count for HTTP requests and TCP proxy (envoyproxy#17583) Signed-off-by: Garrett Bourg <bourg@squareup.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
5 participants
Commit Message: stream info - add upstream connection id
Additional Description: Envoy Mobile is interested in surfacing some information from Stream Info up to the client for analysis purposes. It is useful to understand what streams were multiplexed over the same upstream connection and Envoy's internal connection id can be used to tie this up.
Risk Level:
Testing: pending
Signed-off-by: Jose Nino jnino@lyft.com