telemetry: use native stats by default#41991
Conversation
Signed-off-by: Kuat Yessenov <[email protected]>
istio-testing
added
the
size/XL
kyessenov
added
the
release-notes-none
howardjohn
left a comment
howardjohn
left a comment
There was a problem hiding this comment.
LGTM just wanted to verify - are all the overrides possible compatible with native vs WASM? Or are there some obscure parts of the config that are not implemented/compatible?
|
@howardjohn What exactly do you mean by overrides? Some CEL expressions that refer to Wasm attributes are not compatible, because it's not Wasm. Those are not documented, so unlikely to actually be in-use. |
|
I mean the |
Signed-off-by: Kuat Yessenov <[email protected]>
|
The schema is the same for wasm and stats filter, but I marked some fields removed. Seems like istio fails on the removed xDS fields, so we need to set the flag. |
|
/retest |
1 similar comment
|
/retest |
|
/retest |
1 similar comment
|
/retest |
|
@zirain argh, another metric for istio version needs to be added. not sure why this test fails now only. |
IC istioctl x es sleep-69cfb4968f-h72rz | grep .istio_build
wasmcustom.component.proxy.tag.1.15.3;.istio_build: 1 |
|
/retest |
hzxuzhonghu
left a comment
hzxuzhonghu
left a comment
There was a problem hiding this comment.
@kyessenov Why do we still need the envoyfilter? Couldn't move it to native implement?
|
@hzxuzhonghu We may not need this if #37645 gets done, but noone is assigned to that issue, so until then, this will give us some burn time with EnvoyFilters. |
| // than the flush interval. | ||
| "--file-flush-interval-msec", "1000", | ||
| "--disable-hot-restart", // We don't use it, so disable it to simplify Envoy's logic | ||
| "--allow-unknown-static-fields", |
There was a problem hiding this comment.
Why was this changed? Are there unknown fields now that we are accepting?
There was a problem hiding this comment.
Yeah, because Wasm used a JSON parser that ignored fields with typos so I'm fairly sure some metric overrides had ill-defined fields.
There was a problem hiding this comment.
Why "static" field flags affects ECDS is still not clear to me, likely a bug somewhere.
There was a problem hiding this comment.
I am a bit worried we are going to miss real bugs now for this small telemetry case. But at the same time it would not be great if users with broken overrides pod's don't startup when they upgrade.
On the other hand, they would probably immediately find the issue and rollback/fix I would imagine if we did hard fail...
There was a problem hiding this comment.
We still get a warning for unknown fields.
I think we have to support unknown fields for automatic upgrades, there's just no other way to introduce new fields without putting conditions everywhere.
There was a problem hiding this comment.
Well this is only for static config, right? So over XDS we are fine; static is tightly coupled with the Envoy version since the bootsrtap is generated by pilot-agent which is in the same container... except now we have bootstrap DS so maybe this makes sense now.
I am a bit confused how this dynamic XDS is impacted by allow-unknown-static-fields which I thought was only for startup bootstrap config
There was a problem hiding this comment.
I am a bit confused how this dynamic XDS is impacted by
allow-unknown-static-fieldswhich I thought was only for startup bootstrap config
Me, too, it's probably a bug but it's already shipped so we have to live with it.
There was a problem hiding this comment.
Fair enough. If its a bug might be worth filing an envoy issue? but this lgtm for now - even with it fixed, for Bootstrap DS we may want it.
* telemetry: use native stats by default Signed-off-by: Kuat Yessenov <[email protected]> * removed fields Signed-off-by: Kuat Yessenov <[email protected]> Signed-off-by: Kuat Yessenov <[email protected]>
* update envoy Signed-off-by: Kuat Yessenov <[email protected]> * fix Signed-off-by: Kuat Yessenov <[email protected]> * fixes Signed-off-by: Kuat Yessenov <[email protected]> * test Signed-off-by: Kuat Yessenov <[email protected]> * telemetry: use native stats by default (#41991) * telemetry: use native stats by default Signed-off-by: Kuat Yessenov <[email protected]> * removed fields Signed-off-by: Kuat Yessenov <[email protected]> Signed-off-by: Kuat Yessenov <[email protected]> * fix Signed-off-by: Kuat Yessenov <[email protected]> * make gen Signed-off-by: Kuat Yessenov <[email protected]> Signed-off-by: Kuat Yessenov <[email protected]>
5 participants
Signed-off-by: Kuat Yessenov [email protected]
Follow up to #41441 but for non-telemetry API installation (default), until a plan to default to telemetry API emerges. This should give sufficient burn time in CI, and avoid support for rarely used Wasm version.