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

xray tracer: correctly annotate child spans as xray subsegments #20170

Merged
merged 16 commits into from
Mar 15, 2022
Merged

xray tracer: correctly annotate child spans as xray subsegments #20170

merged 16 commits into from
Mar 15, 2022

Conversation

rexnp
Copy link
Contributor

@rexnp rexnp commented Mar 2, 2022

Commit Message:
In Xray tracing, there is the concept of an xray segment and each segment can contain subsegments. The segment and subsegment maps to Envoy's parent and child spans. In the existing xray extension implementation, each span is emitted independently without the distinction whether the span is a child or not, so Xray treats each span as its own segment.

Whereas in fact, the Xray extension should annotate child spans as subsegments by setting a key value pair "type=subsegment" in the segment payload: https://docs.aws.amazon.com/xray/latest/devguide/xray-api-segmentdocuments.html#api-segmentdocuments-subsegments. This PR is to perform this annotation so Xray subsegments are correctly related to their parent segment.

Additional Description: N/A
Risk Level: low
Testing: unit tests, manual setup using https://github.com/aws/aws-app-mesh-examples/tree/main/walkthroughs/howto-ecs-basics that exercises Xray integration with Envoy.
Docs Changes: Added this change to version history.
Release Notes: Added this change to version history.
Platform Specific Features: N/A
[Optional Fixes #Issue] This will partially address aws/aws-app-mesh-roadmap#354

@rexnp rexnp requested a review from mattklein123 as a code owner March 2, 2022 00:46
@rexnp rexnp mentioned this pull request Mar 2, 2022
Open
@rexnp
Copy link
Contributor Author

rexnp commented Mar 2, 2022

CI failing in presubmit due to spell check. adding the terms Xray and subsegment to the dictionary.

@rojkov
Copy link
Member

rojkov commented Mar 2, 2022

Thank you for fixing it!

BTW, you can exempt a word from spellchecking with backquotes.

/assign @abaptiste
/wait
on CI

@repokitteh-read-only repokitteh-read-only bot added deps Approval required for changes to Envoy's external dependencies and removed waiting labels Mar 2, 2022
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @wrowe

🐱

Caused by: #20170 was synchronize by rexnp.

see: more, trace.

@rexnp rexnp changed the base branch from main to dependabot/github_actions/actions/checkout-3 March 2, 2022 18:22
@rexnp rexnp changed the base branch from dependabot/github_actions/actions/checkout-3 to main March 2, 2022 18:22
alyssawilk and others added 12 commits March 2, 2022 18:28
@alyssawilk @rexnp
test: adding a multi-envoy test (envoyproxy#20016)
d63fec0 
Functionally this

handles the multi-envoy signal handler crash
skips instantiating a runtime singleton (off by default, must stay off until remove global runtime: rebase runtime features on ABSL_flags envoyproxy#19847 is closed)
Multi-envoy does not correctly support runtime flags or deprecation stats due to envoyproxy#19847 being incomplete. It can still handle proxy traffic client - L1 - L2 - upstream as shown in test.

Risk Level: low
Testing: yes
Docs Changes: n/a
Release Notes: n/a
Part of envoyproxy/envoy-mobile#2003

Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Rex Chang <[email protected]>
@wu-bin @rexnp
Add a congestionWindowInBytes method to Envoy::Network::Connection (e…
6a86a26 
…nvoyproxy#20105)

Signed-off-by: Bin Wu <[email protected]>
Signed-off-by: Rex Chang <[email protected]>
@alyssawilk @rexnp
Update QUICHE from 50f15e7a5 to cf1588207 (envoyproxy#20154)
e7f53dd 
https://github.com/google/quiche/compare/50f15e7a5..cf1588207

$ git log 50f15e7a5..cf1588207 --date=short --no-merges --format="%ad %al %s"

2022-02-28 wub Deprecate --gfe2_reloadable_flag_quic_crypto_noop_if_disconnected_after_process_chlo.
2022-02-27 vasilvv Remove QuicheMemSlice(QuicUniqueBufferPtr, size_t) constructor.
2022-02-26 fayang Use std::string instead of absl::string_view in CryptoBufferMap.
2022-02-25 bnc Ignore incoming HTTP/3 MAX_PUSH_ID frames.
2022-02-25 bnc Remove Http3DebugVisitor::OnMaxPushIdFrameSent().
2022-02-25 bnc Remove QuicSpdySession::CanCreatePushStreamWithId().
2022-02-25 fayang Deprecate gfe2_reloadable_flag_quic_single_ack_in_packet2.

Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Rex Chang <[email protected]>
@dependabot @rexnp
build(deps): bump actions/stale from 4.1.0 to 5 (envoyproxy#20159)
d25664f 
Bumps [actions/stale](https://github.com/actions/stale) from 4.1.0 to 5.
- [Release notes](https://github.com/actions/stale/releases)
- [Changelog](https://github.com/actions/stale/blob/main/CHANGELOG.md)
- [Commits](actions/stale@v4.1.0...v5)

---
updated-dependencies:
- dependency-name: actions/stale
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Rex Chang <[email protected]>
@jmarantz @rexnp
admin: improve test coverage and increase the coverage-percent thresh…
f176746 
…old (envoyproxy#20025)

Adds a missing test for recent lookups now that there are no more fake symbol tables. Adds tests for a variety of override methods defined in admin.h that were previously hard to hit.

Adds a benchmark test to establish a baseline for the speedups in envoyproxy#19693

Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Rex Chang <[email protected]>
@alyssawilk @rexnp
test: removing a bunch of direct runtime singleton access (envoyproxy…
fdec215 
…#19993)

Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Rex Chang <[email protected]>
@dependabot @rexnp
build(deps): bump grpcio-tools in /examples/grpc-bridge/client (envoy…
fb6f157 
…proxy#20040)

Bumps [grpcio-tools](https://github.com/grpc/grpc) from 1.43.0 to 1.44.0.
- [Release notes](https://github.com/grpc/grpc/releases)
- [Changelog](https://github.com/grpc/grpc/blob/master/doc/grpc_release_schedule.md)
- [Commits](grpc/grpc@v1.43.0...v1.44.0)

---
updated-dependencies:
- dependency-name: grpcio-tools
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Rex Chang <[email protected]>
@rexnp
adds to spellcheck
c120667 
Signed-off-by: Rex Chang <[email protected]>
@rexnp
xray tracer: set subsegment type for child spans (#2)
7e8134f 
* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <[email protected]>

* adds test coverage

Signed-off-by: Rex Chang <[email protected]>
@rexnp
Xray subsegment (#3)
607c763 
* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <[email protected]>

* adds test coverage

Signed-off-by: Rex Chang <[email protected]>

* updates xray subsegment name to use operation name (instead of parent's span name)

Signed-off-by: Rex Chang <[email protected]>

* updates doc

Signed-off-by: Rex Chang <[email protected]>

* updates doc

Signed-off-by: Rex Chang <[email protected]>
@rexnp
Xray subsegment (#4)
491abf1 
* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <[email protected]>

* adds test coverage

Signed-off-by: Rex Chang <[email protected]>

* updates xray subsegment name to use operation name (instead of parent's span name)

Signed-off-by: Rex Chang <[email protected]>

* updates doc

Signed-off-by: Rex Chang <[email protected]>

* updates doc

Signed-off-by: Rex Chang <[email protected]>

* adds to spell check dictionary

Signed-off-by: Rex Chang <[email protected]>
@rexnp
fixes spellcheck
5be2062 
Signed-off-by: Rex Chang <[email protected]>
@rexnp rexnp changed the base branch from main to dependabot/github_actions/actions/checkout-3 March 2, 2022 18:40
@rexnp rexnp changed the base branch from dependabot/github_actions/actions/checkout-3 to main March 2, 2022 18:40
@rexnp
adds to spellcheck
a26b9b9 
Signed-off-by: Rex Chang <[email protected]>

xray tracer: set subsegment type for child spans (#2)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <[email protected]>

* adds test coverage

Signed-off-by: Rex Chang <[email protected]>

Xray subsegment (#3)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <[email protected]>

* adds test coverage

Signed-off-by: Rex Chang <[email protected]>

* updates xray subsegment name to use operation name (instead of parent's span name)

Signed-off-by: Rex Chang <[email protected]>

* updates doc

Signed-off-by: Rex Chang <[email protected]>

* updates doc

Signed-off-by: Rex Chang <[email protected]>

Xray subsegment (#4)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <[email protected]>

* adds test coverage

Signed-off-by: Rex Chang <[email protected]>

* updates xray subsegment name to use operation name (instead of parent's span name)

Signed-off-by: Rex Chang <[email protected]>

* updates doc

Signed-off-by: Rex Chang <[email protected]>

* updates doc

Signed-off-by: Rex Chang <[email protected]>

* adds to spell check dictionary

Signed-off-by: Rex Chang <[email protected]>

fixes spellcheck

Signed-off-by: Rex Chang <[email protected]>
@rexnp
Merge branch 'envoyproxy:main' into main
cee9167
@rexnp
Merge branch 'main' of github.com:rexnp/envoy
7a485f3 
Signed-off-by: Rex Chang <[email protected]>
@abaptiste
Copy link
Contributor

LGTM. Thanks @rexnp!

wrowe
wrowe suggested changes Mar 10, 2022
View reviewed changes
Copy link
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

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

If you could quickly mop up spelling_dictionary.txt and the associated comments, this is looking about ready to merge.

tools/spelling/spelling_dictionary.txt
@@ -1176,6 +1178,7 @@ subnets
suboptimal
subsecond
subseconds
subsegment
Copy link
Contributor

Choose a reason for hiding this comment

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

M-W agrees this is valid.

tools/spelling/spelling_dictionary.txt Outdated
@@ -401,7 +401,9 @@ XFF
XML
XN
XNOR
XRay
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look right, https://docs.aws.amazon.com/xray/latest/devguide/ spells this X-Ray as the canonical name, no?

tools/spelling/spelling_dictionary.txt Outdated
XSS
Xray
Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely a problem, we shouldn't be using 3+ different terms. But if we were agnostic about exchanging these so liberally, then refer to lines 1-4 of this file which describe how case of the terms affects variant capitalization. "XRAY" would permit any casing and would be preferred to having multiple entries. Using one proper term consistently is preferred of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @wrowe, thanks for taking a look this! my mistake for adding duplicate xray entries to the dictionary. I realize I should just fix the terms XRay to X-Ray where they were violating the spell check so XRAY doesn't even have to be in the dictionary anymore.

Copy link
Contributor

@wrowe wrowe Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Fantastic, I'm not back till Tuesday but if this is cleaned up, it seems like we are good to go. Someone else can also perhaps step up and merge it before then. (Or I can merge in the next few hours.)

@rexnp
fixes spell check
83e97f8 
Signed-off-by: Rex Chang <[email protected]>
wrowe
wrowe approved these changes Mar 15, 2022
View reviewed changes
Copy link
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

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

Thank you for your submission @rexnp, and for fixing the wording! Thank you @abaptiste for the review! LGTM

@wrowe wrowe merged commit 82c02b7 into envoyproxy:main Mar 15, 2022
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
@rexnp @alyssawilk
@wu-bin @dependabot @jmarantz @ravenblackx
xray tracer: correctly annotate child spans as xray subsegments (envo…
593dd2b 
…yproxy#20170)

* test: adding a multi-envoy test (envoyproxy#20016)

Functionally this

handles the multi-envoy signal handler crash
skips instantiating a runtime singleton (off by default, must stay off until remove global runtime: rebase runtime features on ABSL_flags envoyproxy#19847 is closed)
Multi-envoy does not correctly support runtime flags or deprecation stats due to envoyproxy#19847 being incomplete. It can still handle proxy traffic client - L1 - L2 - upstream as shown in test.

Risk Level: low
Testing: yes
Docs Changes: n/a
Release Notes: n/a
Part of envoyproxy/envoy-mobile#2003

Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Rex Chang <[email protected]>

* Add a congestionWindowInBytes method to Envoy::Network::Connection (envoyproxy#20105)

Signed-off-by: Bin Wu <[email protected]>
Signed-off-by: Rex Chang <[email protected]>

* Update QUICHE from 50f15e7a5 to cf1588207 (envoyproxy#20154)

https://github.com/google/quiche/compare/50f15e7a5..cf1588207

$ git log 50f15e7a5..cf1588207 --date=short --no-merges --format="%ad %al %s"

2022-02-28 wub Deprecate --gfe2_reloadable_flag_quic_crypto_noop_if_disconnected_after_process_chlo.
2022-02-27 vasilvv Remove QuicheMemSlice(QuicUniqueBufferPtr, size_t) constructor.
2022-02-26 fayang Use std::string instead of absl::string_view in CryptoBufferMap.
2022-02-25 bnc Ignore incoming HTTP/3 MAX_PUSH_ID frames.
2022-02-25 bnc Remove Http3DebugVisitor::OnMaxPushIdFrameSent().
2022-02-25 bnc Remove QuicSpdySession::CanCreatePushStreamWithId().
2022-02-25 fayang Deprecate gfe2_reloadable_flag_quic_single_ack_in_packet2.

Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Rex Chang <[email protected]>

* build(deps): bump actions/stale from 4.1.0 to 5 (envoyproxy#20159)

Bumps [actions/stale](https://github.com/actions/stale) from 4.1.0 to 5.
- [Release notes](https://github.com/actions/stale/releases)
- [Changelog](https://github.com/actions/stale/blob/main/CHANGELOG.md)
- [Commits](actions/stale@v4.1.0...v5)

---
updated-dependencies:
- dependency-name: actions/stale
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Rex Chang <[email protected]>

* admin: improve test coverage and increase the coverage-percent threshold (envoyproxy#20025)

Adds a missing test for recent lookups now that there are no more fake symbol tables. Adds tests for a variety of override methods defined in admin.h that were previously hard to hit.

Adds a benchmark test to establish a baseline for the speedups in envoyproxy#19693

Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Rex Chang <[email protected]>

* test: removing a bunch of direct runtime singleton access (envoyproxy#19993)

Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Rex Chang <[email protected]>

* build(deps): bump grpcio-tools in /examples/grpc-bridge/client (envoyproxy#20040)

Bumps [grpcio-tools](https://github.com/grpc/grpc) from 1.43.0 to 1.44.0.
- [Release notes](https://github.com/grpc/grpc/releases)
- [Changelog](https://github.com/grpc/grpc/blob/master/doc/grpc_release_schedule.md)
- [Commits](grpc/grpc@v1.43.0...v1.44.0)

---
updated-dependencies:
- dependency-name: grpcio-tools
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Rex Chang <[email protected]>

* adds to spellcheck

Signed-off-by: Rex Chang <[email protected]>

* xray tracer: set subsegment type for child spans (#2)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <[email protected]>

* adds test coverage

Signed-off-by: Rex Chang <[email protected]>

* Xray subsegment (#3)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <[email protected]>

* adds test coverage

Signed-off-by: Rex Chang <[email protected]>

* updates xray subsegment name to use operation name (instead of parent's span name)

Signed-off-by: Rex Chang <[email protected]>

* updates doc

Signed-off-by: Rex Chang <[email protected]>

* updates doc

Signed-off-by: Rex Chang <[email protected]>

* Xray subsegment (#4)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <[email protected]>

* adds test coverage

Signed-off-by: Rex Chang <[email protected]>

* updates xray subsegment name to use operation name (instead of parent's span name)

Signed-off-by: Rex Chang <[email protected]>

* updates doc

Signed-off-by: Rex Chang <[email protected]>

* updates doc

Signed-off-by: Rex Chang <[email protected]>

* adds to spell check dictionary

Signed-off-by: Rex Chang <[email protected]>

* fixes spellcheck

Signed-off-by: Rex Chang <[email protected]>

* adds to spellcheck

Signed-off-by: Rex Chang <[email protected]>

xray tracer: set subsegment type for child spans (#2)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <[email protected]>

* adds test coverage

Signed-off-by: Rex Chang <[email protected]>

Xray subsegment (#3)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <[email protected]>

* adds test coverage

Signed-off-by: Rex Chang <[email protected]>

* updates xray subsegment name to use operation name (instead of parent's span name)

Signed-off-by: Rex Chang <[email protected]>

* updates doc

Signed-off-by: Rex Chang <[email protected]>

* updates doc

Signed-off-by: Rex Chang <[email protected]>

Xray subsegment (#4)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <[email protected]>

* adds test coverage

Signed-off-by: Rex Chang <[email protected]>

* updates xray subsegment name to use operation name (instead of parent's span name)

Signed-off-by: Rex Chang <[email protected]>

* updates doc

Signed-off-by: Rex Chang <[email protected]>

* updates doc

Signed-off-by: Rex Chang <[email protected]>

* adds to spell check dictionary

Signed-off-by: Rex Chang <[email protected]>

fixes spellcheck

Signed-off-by: Rex Chang <[email protected]>

* fixes spell check

Signed-off-by: Rex Chang <[email protected]>

Co-authored-by: alyssawilk <[email protected]>
Co-authored-by: Bin Wu <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Joshua Marantz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wrowe wrowe wrowe approved these changes

@mattklein123 mattklein123 Awaiting requested review from mattklein123 mattklein123 is a code owner

@dio dio Awaiting requested review from dio

@lizan lizan Awaiting requested review from lizan

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

@rojkov rojkov Awaiting requested review from rojkov

@antoniovicente antoniovicente Awaiting requested review from antoniovicente

Assignees

@wrowe wrowe

@abaptiste abaptiste

Labels
deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rexnp @rojkov @abaptiste @wrowe @alyssawilk @wu-bin @jmarantz