wasm: refactor around stats, events and WasmExtension class. by mathetake · Pull Request #17268 · envoyproxy/envoy

mathetake · 2021-07-08T07:51:32Z

This PR refactors the Wasm codebase around host defined stats, and WasmEvent, and WasmExtension class. Notablely, we delete the WasmExtension and EnvoyExtension classes which seem unused and unnecessary abstraction, and add CreateStatsHandler and LifecycleStatsHandler classes for handling Events and host defined metrics. The primary purpose is to drive the future work for the #16726 and #16403.

Signed-off-by: Takeshi Yoneda takeshi@tetrate.io

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake · 2021-07-08T11:20:38Z

/retest

repokitteh-read-only · 2021-07-08T11:20:41Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17268 (comment) was created by @mathetake.

see: more, trace.

PiotrSikora

This is actually used by Istio to export more stats. See:

Having said that, it seems that those stats all refer to async fetch, which we don't use anymore, so perhaps it could be removed... cc @mandarjog @kyessenov @bianpengyuan

mathetake · 2021-07-11T02:11:16Z

Thanks @PiotrSikora, I should have looked at istio/proxy, but yeah as you said, it looks like a redundant and not used anymore.

Also I realized that we could remove IstioContext there as well which I think is the reason for why factories are exposed to Istio via EnvoyWasm class and also why class IstioWasm exists there.

I am more than happy to refactor istio/proxy as well since this could bring benefits both to Envoy and Istio/proxy codebase.

mandarjog · 2021-07-11T06:29:48Z

We can get rid of async fetch, it is not used anymore.
@yuval-k do you use async fetch ?

PiotrSikora · 2021-07-11T21:27:41Z

We can get rid of async fetch, it is not used anymore.

That's a bit of a stretch. The fact that Istio doesn't use it anymore doesn't mean that other Envoy users are not using it.

In any case, if we want to have this discussion, could you please open a new issue? It's a breaking change that shouldn't be discussed in comments of a "random" PR.

mandarjog · 2021-07-11T21:36:04Z

@PiotrSikora agreed wrt Istio, so that cannot be the reason.

however the async fetch is racy. @kyessenov @bianpengyuan can we comment on why we decided that we cannot recommend async fetch alone?

PiotrSikora

LGTM, sans nits.

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake · 2021-07-12T02:31:12Z

I found the coverage flakiness on Context::decodeData and encodeData in context.cc: https://github.com/envoyproxy/envoy/blob/main/source/extensions/common/wasm/context.cc#L1723

OK: https://storage.googleapis.com/envoy-pr/f4e1e1e/coverage/source/extensions/common/wasm/context.cc.gcov.html
Fail: https://storage.googleapis.com/envoy-pr/f413209/coverage/source/extensions/common/wasm/context.cc.gcov.html

retest for now.

mathetake · 2021-07-12T02:31:18Z

/retest

repokitteh-read-only · 2021-07-12T02:31:21Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17268 (comment) was created by @mathetake.

see: more, trace.

mathetake · 2021-07-12T03:27:54Z

/retest

repokitteh-read-only · 2021-07-12T03:27:59Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17268 (comment) was created by @mathetake.

see: more, trace.

mathetake · 2021-07-12T04:06:35Z

hmm.. looks like a deterministic but I have no idea why this affects these lines after merge main..

PiotrSikora · 2021-07-12T04:26:03Z

Code coverage for source/extensions/common is lower than limit of 96.5 (96.4)
Code coverage for source/extensions/common/wasm is lower than limit of 95.3 (95.2)

you need to adjust those values in test/per_file_coverage.sh, or ideally improve the coverage.

mathetake · 2021-07-12T04:39:04Z

yeah but I want to understand why the coverage drops after merging main:

✅ Before main merge: https://storage.googleapis.com/envoy-pr/f4e1e1e/coverage/source/extensions/common/wasm/index.html
❌ After main merge: https://storage.googleapis.com/envoy-pr/f413209/coverage/source/extensions/common/wasm/index.html

I see that some lines of convertFilterDataStatus, encodeData and decodeData has started not being covered after main merge, which seems unrelated to this PR 😞

mathetake · 2021-07-12T11:17:06Z

OK https://github.com/envoyproxy/envoy/pull/17280/files#diff-a2dee6268db4920c2bd9665ba1429c144cd6a9e158b8845c503fb3d5c6fa273cL410-L411 this dropped the coverage on encode/decodeData and convertFilterDataStatus, and this PR triggered the coverage CI failure somehow.

mathetake · 2021-07-12T11:34:38Z

raised the PR to backfill the coverage #17292

PiotrSikora · 2021-07-12T14:00:55Z

OK https://github.com/envoyproxy/envoy/pull/17280/files#diff-a2dee6268db4920c2bd9665ba1429c144cd6a9e158b8845c503fb3d5c6fa273cL410-L411 this dropped the coverage on encode/decodeData and convertFilterDataStatus, and this PR triggered the coverage CI failure somehow.

Oops, good catch! I wonder why the CI didn't catch it in that PR, though.

kyessenov · 2021-07-12T17:40:46Z

Async fetch has no error recovery. If the retries fail and xDS is ACKed, the error is permanent. So it is not recommended unless the network is reliable (which is never true).

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

repokitteh-read-only · 2021-07-13T23:02:46Z

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).

🐱

Caused by: #17268 was synchronize by mathetake.

see: more, trace.

PiotrSikora

Thanks!

moderation · 2021-07-14T00:47:34Z

/lgtm deps

yuval-k · 2021-07-14T17:09:00Z

We can get rid of async fetch, it is not used anymore.
@yuval-k do you use async fetch ?

we also are moving to ECDS, but are using async fetch in the interm.

…oxy#17268) Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

wasm: refactor around stats, events and WasmExtension class.

2a7cc9b

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake marked this pull request as ready for review July 8, 2021 08:06

mathetake requested review from PiotrSikora and lizan as code owners July 8, 2021 08:06

fix order

f4e1e1e

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mattklein123 assigned PiotrSikora Jul 8, 2021

PiotrSikora suggested changes Jul 9, 2021

View reviewed changes

mathetake mentioned this pull request Jul 11, 2021

wasm: delete unused code. istio/proxy#3404

Merged

PiotrSikora reviewed Jul 11, 2021

View reviewed changes

Comment thread source/extensions/common/wasm/stats_handler.h Outdated

Comment thread source/extensions/common/wasm/stats_handler.cc Outdated

mathetake added 3 commits July 12, 2021 08:59

review: fix styles

f3ad1a7

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

review: fix styles

0080bf5

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

Merge remote-tracking branch 'origin/main' into wasm-clean-up

f413209

Merge remote-tracking branch 'origin/main' into wasm-clean-up

88d3638

mathetake mentioned this pull request Jul 13, 2021

wasm: increase coverage on encode/decodeData. #17292

Merged

mathetake added 2 commits July 14, 2021 07:53

Update Proxy-Wasm C++ host, and fix accordingly.

c0c6476

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

Merge remote-tracking branch 'origin/main' into wasm-clean-up

ad83f94

repokitteh-read-only Bot added the deps Approval required for changes to Envoy's external dependencies label Jul 13, 2021

PiotrSikora approved these changes Jul 13, 2021

View reviewed changes

repokitteh-read-only Bot removed the deps Approval required for changes to Envoy's external dependencies label Jul 14, 2021

mattklein123 merged commit f43e9ef into envoyproxy:main Jul 14, 2021

mathetake deleted the wasm-clean-up branch July 14, 2021 23:12

mathetake mentioned this pull request Jul 15, 2021

wasm: remove unused Istio specific Wasm code under src. istio/proxy#3413

Merged

mathetake mentioned this pull request Sep 14, 2021

Update Envoy SHA to 09-13. istio/proxy#3493

Merged

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

wasm: refactor around stats, events and WasmExtension class. (envoypr…

f1363dd

…oxy#17268) Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

Conversation

mathetake commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mathetake commented Jul 8, 2021

Uh oh!

repokitteh-read-only Bot commented Jul 8, 2021

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

mathetake commented Jul 11, 2021

Uh oh!

mandarjog commented Jul 11, 2021

Uh oh!

PiotrSikora commented Jul 11, 2021

Uh oh!

mandarjog commented Jul 11, 2021

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mathetake commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mathetake commented Jul 12, 2021

Uh oh!

repokitteh-read-only Bot commented Jul 12, 2021

Uh oh!

mathetake commented Jul 12, 2021

Uh oh!

repokitteh-read-only Bot commented Jul 12, 2021

Uh oh!

mathetake commented Jul 12, 2021

Uh oh!

PiotrSikora commented Jul 12, 2021

Uh oh!

mathetake commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mathetake commented Jul 12, 2021

Uh oh!

mathetake commented Jul 12, 2021

Uh oh!

PiotrSikora commented Jul 12, 2021

Uh oh!

kyessenov commented Jul 12, 2021

Uh oh!

repokitteh-read-only Bot commented Jul 13, 2021

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

moderation commented Jul 14, 2021

Uh oh!

yuval-k commented Jul 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mathetake commented Jul 8, 2021 •

edited

Loading

mathetake commented Jul 12, 2021 •

edited

Loading

mathetake commented Jul 12, 2021 •

edited

Loading