-
Notifications
You must be signed in to change notification settings - Fork 91
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.
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
Introduce the ability to specify workload type for GetPosixStreamSignals
#391
Introduce the ability to specify workload type for GetPosixStreamSignals
#391
Conversation
api/v0/observe/observe.proto
Outdated
message GetPosixSignalsStreamRequest { | ||
/// The scope of this request. If no scope is specified, a stream of all POXIS | ||
/// signals on the host will be returned. | ||
oneof workload { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might generate nicer code as an enum and an opaque string "id". oneof is pretty nasty, especially in some languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could introduce a "Workload" message with an enum and a string "id" and have this repeated. I need to think this thru a bit though what this means in terms of implementation details for this specific endpoint, as it could get pretty tricky (and not very intuitive) to aggregate POSIX signals for a mix of cells, containers, vms etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we start with the ability to stream signals for a single workload, we can potentially layer the ability to capture signals from multiple workloads on top in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about how the API looks now @dmah42? I will create a separate issue to discuss observing multiple workloads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if we want to avoid using oneof
b/c of how this gets compiled in different languages, should we capture this in the stdlib docs as a "general rule"? https://aurae.io/stdlib/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably point to https://google.aip.dev/ and specifically https://google.aip.dev/search?q=oneof which doesn't say avoid it, but it does provide some trade-offs.
5d9070f
to
71903b9
Compare
message GetPosixSignalsStreamRequest { | ||
/// The workload to which te response will be scoped. If no workload is | ||
/// specified, a stream of all POSIX signals on the host will be returned. | ||
Workload workload = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i personally prefer this (with notes) but i think it should be repeated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we iterate on the API later? Making this repeated
warrants some discussion imo. Like how would the response look like in these cases? For a single workload, the response is just a stream of signals. If you would be able to specify multiple cells/containers etc we would need to return tuples (id, signal). If we allow for a mix of workload types we need to return 3-tuples (workload_type, id, signal) for the user to make sense of the response. Maybe something for an RFC?
d443c4c
to
8ed3d6d
Compare
b535ebf
to
3a771a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just leaving comments. Didn't do a full review.
@@ -0,0 +1 @@ | |||
mod observe; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setup lints
// Lint groups: https://doc.rust-lang.org/rustc/lints/groups.html
#![warn(future_incompatible, nonstandard_style, unused)]
#![warn(
improper_ctypes,
non_shorthand_field_patterns,
no_mangle_generic_items,
unconditional_recursion,
unused_comparisons,
while_true
)]
#![warn(
missing_debug_implementations,
missing_docs,
trivial_casts,
trivial_numeric_casts,
unused_extern_crates,
unused_import_braces,
unused_results
)]
#![warn(clippy::unwrap_used)]
This PR allows for specifying a workload when retrieving the POSIX signals stream. Right now only 1 type of workload is supported (
Cell
) and only a singleCell
can be specified.To achieve this the eBPF probe now surfaces the
cgroup_id
and theobserve_service
has a cache that allows for looking up thecgroup_path
for acgroup_id
. With thecgroup_path
we can filter out signals relevant to a specific cell.