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

upstream: stats scope for dual filters #23103

Merged
merged 5 commits into from
Sep 14, 2022
Merged

Conversation

alyssawilk
Copy link
Contributor

@alyssawilk alyssawilk commented Sep 14, 2022

https://github.com/envoyproxy/envoy/blob/main/source/docs/upstream_filters.md calls out how to convert filters to dual filters and notes that init_manager and stats must come from DualInfo

This pr actually adds stats to DualInfo for use in #23071 and updates docs for filter conversion

Risk Level: low
Testing: updated unit test
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Alyssa Wilk <[email protected]>
@alyssawilk
Copy link
Contributor Author

I'm also going to update the docs in this PR, so the other is a clean example and doesn't include spurious files.

@wbpcode
Copy link
Member

wbpcode commented Sep 14, 2022

Please resolve the conflict in the docs. Thanks.

@wbpcode
Copy link
Member

wbpcode commented Sep 14, 2022

And a minor spelling error in doc:

Either the filter does not access streamInfo in a non-cost way

non-cost -> non-const

@wbpcode
Copy link
Member

wbpcode commented Sep 14, 2022

And could you check following snippet of doc:

  * Add ``UpstreamMyFilterFactory = MyFilterFactory;`` and
    ``DECLARE_FACTORY(UpstreamMyFilterFactory`` in your config.h file and
    ``REGISTER_FACTORY(UpstreamMyFilterFactory,
                     Server::Configuration::UpstreamHttpFilterConfigFactory){"envoy.my_filter"};`` to
    your config.cc file.

Seems it lost something like using or );.

And if DECLARE_FACTORY is necessary? I didn't find it in the #23071.

Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]>
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@alyssawilk alyssawilk enabled auto-merge (squash) September 14, 2022 15:20
@alyssawilk alyssawilk merged commit 17cd512 into envoyproxy:main Sep 14, 2022
@alyssawilk alyssawilk deleted the scope branch March 20, 2023 14:38
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.

2 participants