Metrics for golang filter don't clean when filer removed from LDS configurations

[wonderflow]

If you are reporting any crash or any potential security issue, do not
open an issue in this repo. Please report the issue via emailing
[email protected] where the issue will be triaged appropriately.

Title: Metrics for golang filter don't clean when filer remove from LDS configurations

Description:

What issue is being seen? Describe what should be happening instead of
the bug, for example: Envoy should not crash, the expected value isn't
returned, etc.

When I'm implementing golang filters with metrics, I found the metrics defined under LDS http_connection_manager via ConfigCallback (https://github.com/envoyproxy/envoy/blob/main/contrib/golang/common/go/api/filter.go#L312 ) is a global metrics.

I expect it to:

1. Have listener as metrics name, e.g. "listener.0.0.0.0_19001.opa_total_requests: 0".
2. Will be removed and reset along with filter configurations.

But the real behavior is:

1. The metrics name is just what we defined in the function: "opa_total_requests: 0"
2. The metrics is always there even when I remove the go filter, when I added it back, the counter number is still the old value which not being reset.

This seems to me is a potential bug that causing memory leak.

Repro steps:

Include sample requests, environment, etc. All data and inputs
required to reproduce the bug.

I'm using go filter via httn, the way I used is described in this PR: mosn/htnn#827

After plugin built, what I'm doing is:

1. Update LDS with new golang filter that registered metrics via ConfigCallback.
2. Request the filter related LDS and corresponding path to generate metrics.
3. Check ":/status" to see metrics
4. Remove the golang filter from LDS.
5. Check the metrics that we can see the metrics is still there.
6. Add golang filter back, requesting, we were able to see the metrics are increasing again based on the old value.

Note: The Envoy_collect tool
gathers a tarball with debug logs, config and the following admin
endpoints: /stats, /clusters and /server_info. Please note if there are
privacy concerns, sanitize the data prior to sharing the tarball/pasting.

Admin and Stats Output:

Include the admin output for the following endpoints: /stats,
/clusters, /routes, /server_info. For more information, refer to the
admin endpoint documentation.

Note: If there are privacy concerns, sanitize the data prior to
sharing.

Config:

Include the config used to configure Envoy.

Logs:

Include the access logs and the Envoy logs.

Note: If there are privacy concerns, sanitize the data prior to
sharing.

Call Stack:

If the Envoy binary is crashing, a call stack is required.
Please refer to the Bazel Stack trace documentation.

[KBaichoo]

cc @doujiang24

[doujiang24]

1. Have listener as metrics name, e.g. "listener.0.0.0.0_19001.opa_total_requests: 0".

okay, inherit the stat_prefix seems more reasonable.

2. Will be removed and reset along with filter configurations.

Seems there isn't a way remove a stats.

@StarryVae Could you please take a look?

[StarryVae]

1. Have listener as metrics name, e.g. "listener.0.0.0.0_19001.opa_total_requests: 0".

i also think it is more reasonable to inherit the stat_prefix, and i will submit a PR to support it.

2. Will be removed and reset along with filter configurations.

AFAIK, the metric is just removed with LDS update when you remove the golang filter, and there will be a new metric when you bring the filter back, maybe you can try to wait for several time like 30s or more to ensure the old golang filter config is deleted by envoy. cc @wonderflow

[wonderflow]

@doujiang24 @StarryVae Thanks for helping.

the metric is just removed with LDS update when you remove the golang filter

The issue is not only when I removed golang filter with LDS and metrics is still there, but also when I add the golang filter back with a second LDS updates, the metrics number increased from the old one instead of from zero.

[shubham1172]

i also think it is more reasonable to inherit the stat_prefix, and i will submit a PR to support it.

Thanks @StarryVae, I have run into the same issue with WASM metrics. There are no prefixes today and the metric name is as is. Enriching it with some runtime information will be helpful. Let me know if I should create another issue to track it, happy to do it.