search

p4lang / p4-applications

P4 Applications WG repo
107 stars 46 forks source link

Add multiple telemetry reports per packet #76

Closed RandyLevensalor closed 4 years ago

RandyLevensalor commented 4 years ago

This PR adds support for multiple telemetry reports in a single packet.

Since the TR header, INT header and packet header can be a fraction of a frame, especially when jumbo frames are enabled, we can encapsulate multiple reports in one packet.

Networks are frequently constrained by packet rates. This will allow for more reports to be sent without increasing the number of telemetry report packets.

This is just one option to implement this. An additional shim head could also be used instead of modifying the telemetry report header.

jklr commented 4 years ago

@RandyLevensalor I remember that the need to coalesce multiple reports into one pkt were discussed in a previous meeting. I think the conclusion at that time was to leverage the domain extension of the report header to support this. The PR is here: https://github.com/p4lang/p4-applications/pull/61

@rsivakolundu can you pls confirm/correct?

Thanks.

RandyLevensalor commented 4 years ago

@jklr Thanks for sharing that this had been removed from the 2.0 proposal. I only saw the latest version and didn't see it.

I'm interested to hear why this should be relegated to domain-specific capability and would not be a general part of the spec. This would limit this to sink switches that support a P4 or a similar mode of customization. Plus this would need to be added to all domain-specific implementations since only one domain-specific ID is currently supported.

There are other options for how this could be implemented in the spec.

If you consider that this can be implemented on networks that support jumbo frames and the header would just be a small fraction of that frame. The lack of multiple reports per packet increased the number of packets needed by over an order of magnitude and effectively throttles the sampling rate.

Edit: If we go down this domain-specific route, would adding an appendix with an example of this be useful?

jklr commented 4 years ago

@RandyLevensalor we never had the report coalescing feature in the spec. We surely discussed the need in one of the previous meetings, not sure about the conclusion. Let's discuss tomorrow.

RandyLevensalor commented 4 years ago

Added an alternate implementation using domain specific metadata. I'd still prefer this to be in the default telemetry report header, but this is being submitted as an alternate if that is not agreeable.

mickeyspiegel commented 4 years ago

Addressed in a separate PR for Telemetry Report 2.0