Skip to content

service design proposal: batched sampling #24

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
LimeHat wants to merge 1 commit into
base: main
Choose a base branch
from
Open

service design proposal: batched sampling #24

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions proto/gnpsi/gnpsi.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ service gNPSI {
// updates from the device. Past updates, i.e., updates before the
// subscription is received, will not be presented to the subscribing client.
rpc Subscribe(Request) returns (stream Sample);
rpc SubscribeBatch(Request) returns (stream Samples);
}

message SFlowMetadata {
Expand Down Expand Up @@ -86,3 +87,7 @@ message Sample {
NetFlowMetadata netflow_metadata = 102;
IPFIXMetadata ipfix_metadata = 103;
}

message Samples {
repeated Sample samples = 1;

@marcushines marcushines Feb 13, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am not clear why this is considered to be a good change
the problem with adding this option is now you have to worrry about the size of the batches and i am unclear how this signifcantly improves performance?

@LimeHat LimeHat Feb 13, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now you have to worrry about the size of the batches

can you elaborate on that? are you talking about server-side or client-side?

and i am unclear how this signifcantly improves performance?

by default1 sending each message implies a write() syscall which is an expensive operation and in the past we've seen bottlenecks in this area. and only 1 write can be active at a time for each stream. We'd like to have an ability to reduce the number of writes in the server code, and, based on our experience, this allows to achieve better performance.

Footnotes

  1. this is not neccesarily true for all grpc libraries; e.g. in golang, AFAIR, grpc-go performs a transport-level buffering/batching before doing a write() to the socket. but that's not the case with C/C++ grpc lib for example. ↩

@LimeHat LimeHat Feb 14, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an extra note, this is not a novel or unique pattern. In OC world, for example, it is already used with gNMI and gRIBI streaming services

}