-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
enhancement(new sink): new sink for Google chronicle #13550
Conversation
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
✅ Deploy Preview for vector-project canceled.
|
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
|
||
impl GenerateConfig for ChronicleUnstructuredConfig { | ||
fn generate_config() -> toml::Value { | ||
toml::from_str(indoc! {r#" |
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.
Nit: I prefer using Self
here to get completion help, but I don't think we've agreed/determined a standard here.
Ex: https://github.com/vectordotdev/vector/blob/master/src/sinks/websocket/config.rs#L32-L40
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.
Hmmm.. I prefer it this way. It means we can verify that a minimal config looks sane.
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 get a consensus as a team and commit to doing it one way or another.
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'd say we defer that to another issue since that would affect more sinks. Personally I don't have any strong opinions about either style. I can see @StephenWakely's argument, it has helped me a couple of times to catch deserialization issues as well
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.
Yep - 100% not needing to be handled here!
|
||
#[async_trait::async_trait] | ||
#[typetag::serde(name = "gcp_chronicle_unstructured")] | ||
impl SinkConfig for ChronicleUnstructuredConfig { |
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.
Is the unstructured vs structured(?) so different that they can't be a single sink with two encoding options?
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.
It's unstructured vs UDM.
There's an open question still on the RFC that needs resolving. Please chime in here with your thoughts.
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
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 a couple of nit level comments, otherwise happy to unblock users with this.
} | ||
} | ||
log_type: { | ||
description: "Identifies the log entry. This must be one of the supported log types, otherwise Chronicle will reject the entry with an error." |
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.
Do we have a list of supported log types we can share here or at least link to?
Soak Test ResultsBaseline: ce65a86 ExplanationA soak test is an integrated performance test for vector in a repeatable rig, with varying configuration for vector. What follows is a statistical summary of a brief vector run for each configuration across SHAs given above. The goal of these tests are to determine, quickly, if vector performance is changed and to what degree by a pull request. Where appropriate units are scaled per-core. The table below, if present, lists those experiments that have experienced a statistically significant change in their throughput performance between baseline and comparision SHAs, with 90.0% confidence OR have been detected as newly erratic. Negative values mean that baseline is faster, positive comparison. Results that do not exhibit more than a ±8.87% change in mean throughput are discarded. An experiment is erratic if its coefficient of variation is greater than 0.3. The abbreviated table will be omitted if no interesting changes are observed. No interesting changes in throughput with confidence ≥ 90.00% and absolute Δ mean >= ±8.87%: Fine details of change detection per experiment.
|
} | ||
|
||
impl Service<ChronicleRequest> for ChronicleService { | ||
type Response = GcsResponse; |
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 be common knowledge once you get familiar with things but, it took me a bit of digging to realize that component_sent_events_total
and component_sent_event_bytes_total
(required to be emitted by this type of component) , are emitted as part of EventsSent
which is handled by the GcsResponse
/ DriverResponse
Trait function events_sent()
Not sure if it really warrants a comment but, I figured I'd call it out as a newbie.
(Some(endpoint), None) => endpoint.trim_end_matches('/'), | ||
(None, Some(region)) => region.endpoint(), | ||
(Some(_), Some(_)) => return Err(ChronicleError::BothRegionAndEndpoint), | ||
(None, None) => return Err(ChronicleError::RegionOrEndpoint), |
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.
It seems like this logic is configuration based , would it be beneficial to add a unit test case for it ?
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.
Not sure I understand what you mean?
@@ -0,0 +1,526 @@ | |||
//! This sink sends data to Google Chronicles unstructured log entries endpoint. |
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 noticing this is the primary inline documentation for the file (in reference to the "REVIEWING.md" guide).
Though I'm glancing at a few other sink implementations and, not seeing a whole lot of in line documentation in those either 🤷
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
1896d81
to
25416e0
Compare
Signed-off-by: Stephen Wakely <[email protected]>
Soak Test ResultsBaseline: a2cf7d2 ExplanationA soak test is an integrated performance test for vector in a repeatable rig, with varying configuration for vector. What follows is a statistical summary of a brief vector run for each configuration across SHAs given above. The goal of these tests are to determine, quickly, if vector performance is changed and to what degree by a pull request. Where appropriate units are scaled per-core. The table below, if present, lists those experiments that have experienced a statistically significant change in their throughput performance between baseline and comparision SHAs, with 90.0% confidence OR have been detected as newly erratic. Negative values mean that baseline is faster, positive comparison. Results that do not exhibit more than a ±8.87% change in mean throughput are discarded. An experiment is erratic if its coefficient of variation is greater than 0.3. The abbreviated table will be omitted if no interesting changes are observed. No interesting changes in throughput with confidence ≥ 90.00% and absolute Δ mean >= ±8.87%: Fine details of change detection per experiment.
|
Soak Test ResultsBaseline: a3a332f ExplanationA soak test is an integrated performance test for vector in a repeatable rig, with varying configuration for vector. What follows is a statistical summary of a brief vector run for each configuration across SHAs given above. The goal of these tests are to determine, quickly, if vector performance is changed and to what degree by a pull request. Where appropriate units are scaled per-core. The table below, if present, lists those experiments that have experienced a statistically significant change in their throughput performance between baseline and comparision SHAs, with 90.0% confidence OR have been detected as newly erratic. Negative values mean that baseline is faster, positive comparison. Results that do not exhibit more than a ±8.87% change in mean throughput are discarded. An experiment is erratic if its coefficient of variation is greater than 0.3. The abbreviated table will be omitted if no interesting changes are observed. No interesting changes in throughput with confidence ≥ 90.00% and absolute Δ mean >= ±8.87%: Fine details of change detection per experiment.
|
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
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.
Looks good, nice work!
One last suggestion that I'd like to go in:
Soak Test ResultsBaseline: f9023e2 ExplanationA soak test is an integrated performance test for vector in a repeatable rig, with varying configuration for vector. What follows is a statistical summary of a brief vector run for each configuration across SHAs given above. The goal of these tests are to determine, quickly, if vector performance is changed and to what degree by a pull request. Where appropriate units are scaled per-core. The table below, if present, lists those experiments that have experienced a statistically significant change in their throughput performance between baseline and comparision SHAs, with 90.0% confidence OR have been detected as newly erratic. Negative values mean that baseline is faster, positive comparison. Results that do not exhibit more than a ±8.87% change in mean throughput are discarded. An experiment is erratic if its coefficient of variation is greater than 0.3. The abbreviated table will be omitted if no interesting changes are observed. No interesting changes in throughput with confidence ≥ 90.00% and absolute Δ mean >= ±8.87%: Fine details of change detection per experiment.
|
Signed-off-by: Stephen Wakely <[email protected]>
Soak Test ResultsBaseline: f9023e2 ExplanationA soak test is an integrated performance test for vector in a repeatable rig, with varying configuration for vector. What follows is a statistical summary of a brief vector run for each configuration across SHAs given above. The goal of these tests are to determine, quickly, if vector performance is changed and to what degree by a pull request. Where appropriate units are scaled per-core. The table below, if present, lists those experiments that have experienced a statistically significant change in their throughput performance between baseline and comparision SHAs, with 90.0% confidence OR have been detected as newly erratic. Negative values mean that baseline is faster, positive comparison. Results that do not exhibit more than a ±8.87% change in mean throughput are discarded. An experiment is erratic if its coefficient of variation is greater than 0.3. The abbreviated table will be omitted if no interesting changes are observed. No interesting changes in throughput with confidence ≥ 90.00% and absolute Δ mean >= ±8.87%: Fine details of change detection per experiment.
|
Ref #11532
This creates a new sink for Google Chronicle unstructured log entries.
Note, the integration test is running against a dummy http server that I am maintaining here and will document a bit better shortly. The RSA keys used for the authentication of this test have been freshly generated and are not used anywhere else!
This has also been tested against a real Chronicle account.
This is also using a fork of the rust-goauth library, which can be reverted once this pull request is merged.