Skip to content

Update Envoy SHA to 09-13.#3493

Merged
istio-testing merged 4 commits intoistio:masterfrom
mathetake:envoyupdate
Sep 14, 2021
Merged

Update Envoy SHA to 09-13.#3493
istio-testing merged 4 commits intoistio:masterfrom
mathetake:envoyupdate

Conversation

@mathetake
Copy link
Copy Markdown
Member

@mathetake mathetake commented Sep 14, 2021

Thanks to envoyproxy/envoy#17357, registerPrometheusNamespace has been deleted and we no longer need to statically register prometheus stat namespaces. Not only that, this also resolves istio/istio#27635 since the change in Envoy is handling any stat namespaces transparently to users and therefore users are free to determine the namespace they want to use in their own metrics exposed by Wasm plugins.

This completes the refactoring for removing all the Istio specific Wasm code base in Envoy (i.e. C++ code under src/**) combined with #3413 #3404 envoyproxy/envoy#17268

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake mathetake requested a review from a team September 14, 2021 00:47
@google-cla google-cla Bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Sep 14, 2021
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 14, 2021
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake
Copy link
Copy Markdown
Member Author

mathetake commented Sep 14, 2021

Looks like the stackdriver plugin is implicitly relying on the past Envoy's behavior of prefixing all the metrics with "envoy_" .. need to make change to the plugin so that it explicitly prefixes with "envoy_".

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake mathetake requested a review from a team September 14, 2021 02:20
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 14, 2021
Comment on lines +37 to 40
Metric export_call(MetricType::Counter, "envoy_export_call",
{MetricTag{"wasm_filter", MetricTag::TagType::String},
MetricTag{"type", MetricTag::TagType::String},
MetricTag{"success", MetricTag::TagType::Bool}});
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@bianpengyuan is this actually used?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah we occasionally use this to debug that stackdriver export call is made or not.

@mathetake
Copy link
Copy Markdown
Member Author

/retest

@istio-testing istio-testing merged commit 0497b94 into istio:master Sep 14, 2021
@bianpengyuan
Copy link
Copy Markdown
Contributor

@mathetake actually, curious how do we generate istio stats with istio_ prefix now? I'd expect that we will have some change in the stats filter but looks like there is not.

@mathetake mathetake deleted the envoyupdate branch September 14, 2021 23:18
@mathetake
Copy link
Copy Markdown
Member Author

The "istio" prefix has already been added at extensions side:

const std::string default_stat_prefix = "istio";

so we don't need to modify that. Previously what we have done so far is that using registerPrometheusNamespace in the Envoy codebase to register "istio" statically, and make Envoy opt-out "istio**" metrics in the prometheus endpoint, meaning Envoy doesn't append "envoy_" prefix to these "istio" prefixed metrics when they are exposed in /stats/prometheus.

The change in envoyproxy/envoy#17357 removed the need for registerPrometheusNamespace and made the prometheus stat endpoint (/stats/prometheus) expose all the Wasm-program-defined metrics as-is. That means, not only "istio_**" metrics but also any other user-defined custom metrics are exposed in the /stats/prometheus without being modified (which is great for all the Wasm extension users!).

@bianpengyuan
Copy link
Copy Markdown
Contributor

I see, thanks for detailed explanation! @mathetake

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

wasm: stat prefix is hard-coded

3 participants