Skip to content

listener: 2nd attempt create per network filter chain stats scope#18564

Merged
mattklein123 merged 11 commits intoenvoyproxy:mainfrom
lambdai:perfcscopeagain
Oct 15, 2021
Merged

listener: 2nd attempt create per network filter chain stats scope#18564
mattklein123 merged 11 commits intoenvoyproxy:mainfrom
lambdai:perfcscopeagain

Conversation

@lambdai
Copy link
Copy Markdown
Contributor

@lambdai lambdai commented Oct 11, 2021

Commit Message:
This is the 2nd attempt of #17931
The goal is to fix some stats leak described in #17690

The above PR is reverted because the shared grpc logger can reference a destroyed stat scope and that behavior is amplified by the above PR.
The grpc logger issue is fixed by #18067

In case further improper behaviors are detected at a network filter or any extension,
envoy.reloadable_features.per_network_filter_chain_factory_context is added here so we don't have to revert this PR.

Signed-off-by: Yuchen Dai silentdai@gmail.com

Additional Description:
Risk Level: Mid
Testing: Tests and runtime flag behavior test.
Docs Changes:
Release Notes:
Platform Specific Features:
Runtime guard: envoy.reloadable_features.per_network_filter_chain_factory_context
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
@davinci26
Copy link
Copy Markdown
Member

/assign @envoyproxy/first-pass-reviewers

@repokitteh-read-only
Copy link
Copy Markdown

@envoyproxy/first-pass-reviewers cannot be assigned to this issue.

🐱

Caused by: a #18564 (comment) was created by @davinci26.

see: more, trace.

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Awesome thanks. TBH I would skip the runtime flag here, as if people see crashes we are just going to revert this anyway and try again. I don't think it's worth adding the overhead for such a small change. Can we remove that part and just revert the revert cleanly and see if it sticks?

/wait

@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Oct 14, 2021

if people see crashes we are just going to revert this anyway and try again.

Thank you for the feedback. I am not sure if I understand the piece. Isn't the goal of that runtime is to avoid revert, instead, we disable the runtime feature at a smaller penalty than revert.

I am fine and be prepared to revert though.

@mattklein123
Copy link
Copy Markdown
Member

Thank you for the feedback. I am not sure if I understand the piece. Isn't the goal of that runtime is to avoid revert, instead, we disable the runtime feature at a smaller penalty than revert.

Yes, in general, but we don't typically do it for small changes like this. More like large changes where we might not know the implications for a long time, might be difficult to revert, etc. For this, I think we are going to know pretty quickly whether it works or not, so I would recommend just trying the fix again directly without the cleanup overhead?

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Oct 14, 2021

I see. Make sense to me. Thank you for the explanation.

I clean up the runtime related code and test. PTAL

@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Oct 15, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18564 (comment) was created by @lambdai.

see: more, trace.

@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Oct 15, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

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

🐱

Caused by: a #18564 (comment) was created by @lambdai.

see: more, trace.

@jmarantz
Copy link
Copy Markdown
Contributor

/wait

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

/assign-from @envoyproxy/senior-maintainers

@repokitteh-read-only
Copy link
Copy Markdown

@envoyproxy/senior-maintainers assignee is @mattklein123

🐱

Caused by: a #18564 (review) was submitted by @jmarantz.

see: more, trace.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit 14a052a into envoyproxy:main Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants