Metrics not working following 1.5 -> 1.6 upgrade

[Stono]

Bug description
As I've blogged about - it's really important for us that we're able to reduce the cardinality of metrics at the Sidecar.

We did this using an EnvoyFilter configured with:

                {
                  "debug": "false",
                  "stat_prefix": "istio",
                  "metrics": {
                    "tags_to_remove": ["destination_canonical_service", "source_canonical_service", "destination_principal", "source_principal", "connection_security_policy", "grpc_response_status", "source_version", "destination_version", "request_protocol", "source_canonical_revision", "destination_canonical_revision"]
                  },
                  "metrics": {
                    "name": "request_duration_milliseconds",
                    "tags_to_remove": ["response_code", "response_flags"],
                  },
                  "metrics": {
                    "name": "request_bytes",
                    "tags_to_remove": ["response_code", "response_flags"],
                  },
                  "metrics": {
                    "name": "response_bytes",
                    "tags_to_remove": ["response_code", "response_flags"],
                  }
                }

This works fine on 1.5, but on 1.6 we get no metrics at all. Switching it back to the default:

                  {
                    "debug": "false",
                    "stat_prefix": "istio"
                  }

And we start getting metrics, but with all the dimensions we're not interested in.

I'm guessing something has changed about this config between 1.5 and 1.6 but I cannot see anything in documentation to tell me what/why.

It also really highlights how dangerous EnvoyFilters are for configuration rather than first class API's.

cc @howardjohn @douglas-reid

[ ] Docs
[ ] Installation
[ ] Networking
[ ] Performance and Scalability
[x] Extensions and Telemetry
[ ] Security
[ ] Test and Release
[x] User Experience
[x] Developer Infrastructure

Expected behavior
Metrics to work

Steps to reproduce the bug

Version (include the output of istioctl version --remote and kubectl version --short and helm version if you used Helm)
1.6

How was Istio installed?
Helm

Environment where bug was observed (cloud vendor, OS, etc)

[douglas-reid]

@kyessenov did anything change between 1.5 and 1.6 with respect to supporting the repeated MetricsConfig metrics or similar?

[Stono]

Modifying the configuration to:

                {
                  "debug": "false",
                  "stat_prefix": "istio",
                  "metrics": [
                    {
                      "tags_to_remove": ["destination_canonical_service", "source_canonical_service", "destination_principal", "source_principal", "connection_security_policy", "grpc_response_status", "source_version", "destination_version", "request_protocol", "source_canonical_revision", "destination_canonical_revision"]
                    },
                    {
                      "name": "request_duration_milliseconds",
                      "tags_to_remove": ["response_code", "response_flags"],
                    },
                    {
                      "name": "request_bytes",
                      "tags_to_remove": ["response_code", "response_flags"],
                    },
                    {
                      "name": "response_bytes",
                      "tags_to_remove": ["response_code", "response_flags"],
                    }
                  ]
                }

(note the array). Resolved the issue.

@douglas-reid suggested istio/proxy#2807, in particular https://github.com/istio/proxy/pull/2807/files#diff-db83bcb3df7f25cfb88ff0a20bbd5540 was likely the cause.

As a user - this was extremely difficult to debug, and would expect a better UX than just no metrics and a silent failure.

[kyessenov]

@Stono Yes, the fix is right. I am entirely with you on the danger of EnvoyFilters.
RE: schema, I think we should do a better job in istio on enforcing JSON format. Protobuf is very lax so some gray areas like duplicate keys are accepted by protobuf parser, but not other common JSON parsers.

[douglas-reid]

@Stono is this OK to close now?

[Stono]

It depends, do we not want to tackle the UX issue here in some way? It's a breaking change between 1.5 and 1.6 which meant i was left with no metrics and silent failure. 🤷

[douglas-reid]

@kyessenov how hard would it be to write a validator / converter across the parsing regimes ?

[kyessenov]

@douglas-reid Do you mean a validator for EnvoyFilters? There is a long list of tasks to make EnvoyFilter safe between upgrades (see #26627). It's not even possible to do it completely, but we want to carve out a subset that's reasonable.

For this issue specifically, I'm not sure where to put the tool. Istio does not have any custom code for extensions besides EnvoyFilters. The extension itself is not appropriate since we removed protobuf to make it smaller (the parser from protobuf easily takes a megabyte of binary, more than the extension itself).

[mandarjog]

I agree that this was a behaviour change.
@Stono I am curious, do you use proto json encoding elsewhere as well, or was it inadvertent use here?

[Stono]

This is the only place we use it

[ssuchter]

Should metrics not working be possible to consider as a failure, so that canary upgrades would cause a rollback?

[mandarjog]

Yes, In fact a traffic metrics based canary would automatically stop progress when it suddenly cannot find metrics.