Add FIELD accessor support for peer metadata in tracing#6765
Add FIELD accessor support for peer metadata in tracing#6765istio-testing merged 9 commits intoistio:masterfrom
Conversation
Signed-off-by: Petr McAllister <petr.mcallister@gmail.com>
|
can we hold this until we make an agreement of #6748? |
keithmattix
left a comment
There was a problem hiding this comment.
So we're no longer storing a string in filter state but instead a serializable object; how does that change the contract for when we read this out of filter state? Where's the caller and what does it have to change?
Signed-off-by: Petr McAllister <petr.mcallister@gmail.com>
Thanks @keithmattix good catch - istio_stats.cc was reading it as CelState and deserializing back to WorkloadMetadataObject. Updated it to read WorkloadMetadataObject directly with CelState fallback for backward compat. Eliminates the round-trip in the new path bbbf405 does it resolve your concern? |
Signed-off-by: Petr McAllister <petr.mcallister@gmail.com>
Signed-off-by: Petr McAllister <petr.mcallister@gmail.com>
|
/retest |
…ssor support Signed-off-by: Petr McAllister <petr.mcallister@gmail.com>
Signed-off-by: Petr McAllister <petr.mcallister@gmail.com>
|
/retest |
Signed-off-by: Petr McAllister <petr.mcallister@gmail.com>
|
/retest |
|
@keithmattix - had to rework this PR to preserve @zirain - would you consider giving it a green light? Thank you! |
@zirain do you think it would be difficult to sync back the changes from this repo to Envoy if we agree to move the code to Envoy, is that your concern? FWIW, from the WG meeting discussion about #6748 there was a concern about keeping the code in one place and tests in another place. I don't think that we've reached a consensus on how to treat that concern (whether it's blocker or not and how to proceed with that in general). Taking a stance that no PRs can be merged until we agree is a bit too strict, and if there are no major concerns with syncing the repos once we do reach consensus, then it also seems somewhat unnecessary constraint. |
Signed-off-by: Petr McAllister <petr.mcallister@gmail.com>
krinkinmu
left a comment
There was a problem hiding this comment.
LGTM overall. I'm not sure if downstream_peer_obj and upstream_peer_obj names for filter state keys is the best, but I also don't have any particularly good alternatives either.
Might be worth bringing this PR up in WG to:
- Figure out if everyone is on board with the filter state key names (and if somebody could come up with better names)
- Make progress on whether we should block proxy PRs while we are still discussing migration to Envoy.
|
/retest |
|
In response to a cherrypick label: new pull request created: #6775 |
* Add FIELD accessor support for peer metadata in tracing Signed-off-by: Petr McAllister <petr.mcallister@gmail.com> * lint Signed-off-by: Petr McAllister <petr.mcallister@gmail.com> * address PR comments, fix tests Signed-off-by: Petr McAllister <petr.mcallister@gmail.com> * fix test when labels are not present Signed-off-by: Petr McAllister <petr.mcallister@gmail.com> * test fixes Signed-off-by: Petr McAllister <petr.mcallister@gmail.com> * Store both CelState and WorkloadMetadataObject for CEL and FIELD accessor support Signed-off-by: Petr McAllister <petr.mcallister@gmail.com> * Use separate keys for CelState and WorkloadMetadataObject storage Signed-off-by: Petr McAllister <petr.mcallister@gmail.com> * Update istio_stats to use new peer metadata object keys Signed-off-by: Petr McAllister <petr.mcallister@gmail.com> * Changed peerInfoRead to check the *PeerObj Signed-off-by: Petr McAllister <petr.mcallister@gmail.com> --------- Signed-off-by: Petr McAllister <petr.mcallister@gmail.com>
|
@PetrMc what is the diff between the new formatter with |
|
| @PetrMc what is the diff between the new formatter with %CEL(filter_state['upstream_peer'].workload)%? Thanks @zirain - FIELD accessor calls getField() directly on the C++ object - just a string lookup. CEL has to deserialize the protobuf and evaluate the expression each time. Should be faster for per-request tracing tags, though haven't benchmarked it yet. |
|
will this cost more memory in theoretically? |
|
What I want to say is that, we need a bechmark to help us make a tradeoff between memory usage increase(the FILTER_STATE way) and cpu usage(the CEL way). |
|
makes sense @zirain - the two-key approach does add memory overhead. looking at envoy that has substitution_formatter_speed_test.cc: we could add similar benchmarks comparing FIELD accessor vs CEL. open to other approaches if you have preferred benchmarking method. |
substitution_formatter_speed_test sounds good. |
What this PR does / why we need it:
Enables FIELD accessor support for peer metadata to allow waypoint proxies to export source workload tags (istio.source_workload, istio.source_namespace, etc.) in traces. Fixes istio/istio#58348
Stores WorkloadMetadataObject directly in filter state instead of wrapping in CelState. This enables formatters to use %FILTER_STATE(downstream_peer:FIELD:workload)% syntax while maintaining CEL compatibility via serializeAsProto().
Special notes for your reviewer:
this requires envoyproxy/envoy#41697 that is included in Envoy 1.37 (released on Jan 13, 2026) and included in [
release-1.29] of Istio