feat(codecs): Add CEF encoder#17389
Conversation
✅ Deploy Preview for vector-project canceled.
|
✅ Deploy Preview for vrl-playground ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
github-actions
bot
added
domain: codecs
Regression Detector ResultsRun ID: 16b6b5ff-a6ae-40a2-9e45-d224f06a1f18 ExplanationA regression test is an integrated performance test for Because a target's optimization goal performance in each experiment will vary somewhat each time it is run, we can only estimate mean differences in optimization goal relative to the baseline target. We express these differences as a percentage change relative to the baseline target, denoted "Δ mean %". These estimates are made to a precision that balances accuracy and cost control. We represent this precision as a 90.00% confidence interval denoted "Δ mean % CI": there is a 90.00% chance that the true value of "Δ mean %" is in that interval. We decide whether a change in performance is a "regression" -- a change worth investigating further -- if both of the following two criteria are true:
The table below, if present, lists those experiments that have experienced a statistically significant change in mean optimization goal performance between baseline and comparison SHAs with 90.00% confidence OR have been detected as newly erratic. Negative values of "Δ mean %" mean that baseline is faster, whereas positive values of "Δ mean %" mean that comparison is faster. Results that do not exhibit more than a ±5.00% change in their mean optimization goal are discarded. An experiment is erratic if its coefficient of variation is greater than 0.1. The abbreviated table will be omitted if no interesting change is observed. No interesting changes in experiment optimization goals with confidence ≥ 90.00% and |Δ mean %| ≥ 5.00%. Fine details of change detection per experiment.
|
|
Thanks for this @nabokihms ! Just a quick note that the best reviewer for this is on PTO this week, but we'll get this reviewed more thoroughly this upcoming week. |
|
Tagging @neuronull here too since it is somewhat akin to the GELF codec. |
Regression Detector ResultsRun ID: 4b8e05b7-a99d-4edb-8e7c-6c956330d469 ExplanationA regression test is an integrated performance test for Because a target's optimization goal performance in each experiment will vary somewhat each time it is run, we can only estimate mean differences in optimization goal relative to the baseline target. We express these differences as a percentage change relative to the baseline target, denoted "Δ mean %". These estimates are made to a precision that balances accuracy and cost control. We represent this precision as a 90.00% confidence interval denoted "Δ mean % CI": there is a 90.00% chance that the true value of "Δ mean %" is in that interval. We decide whether a change in performance is a "regression" -- a change worth investigating further -- if both of the following two criteria are true:
The table below, if present, lists those experiments that have experienced a statistically significant change in mean optimization goal performance between baseline and comparison SHAs with 90.00% confidence OR have been detected as newly erratic. Negative values of "Δ mean %" mean that baseline is faster, whereas positive values of "Δ mean %" mean that comparison is faster. Results that do not exhibit more than a ±5.00% change in their mean optimization goal are discarded. An experiment is erratic if its coefficient of variation is greater than 0.1. The abbreviated table will be omitted if no interesting change is observed. No interesting changes in experiment optimization goals with confidence ≥ 90.00% and |Δ mean %| ≥ 5.00%. Fine details of change detection per experiment.
|
Regression Detector ResultsRun ID: 0237d43c-f539-48ac-99de-ca14dac27755 ExplanationA regression test is an integrated performance test for Because a target's optimization goal performance in each experiment will vary somewhat each time it is run, we can only estimate mean differences in optimization goal relative to the baseline target. We express these differences as a percentage change relative to the baseline target, denoted "Δ mean %". These estimates are made to a precision that balances accuracy and cost control. We represent this precision as a 90.00% confidence interval denoted "Δ mean % CI": there is a 90.00% chance that the true value of "Δ mean %" is in that interval. We decide whether a change in performance is a "regression" -- a change worth investigating further -- if both of the following two criteria are true:
The table below, if present, lists those experiments that have experienced a statistically significant change in mean optimization goal performance between baseline and comparison SHAs with 90.00% confidence OR have been detected as newly erratic. Negative values of "Δ mean %" mean that baseline is faster, whereas positive values of "Δ mean %" mean that comparison is faster. Results that do not exhibit more than a ±5.00% change in their mean optimization goal are discarded. An experiment is erratic if its coefficient of variation is greater than 0.1. The abbreviated table will be omitted if no interesting change is observed. No interesting changes in experiment optimization goals with confidence ≥ 90.00% and |Δ mean %| ≥ 5.00%. Fine details of change detection per experiment.
|
Regression Detector ResultsRun ID: e86c3f6b-86b0-4f11-b003-86edecbe6b20 ExplanationA regression test is an integrated performance test for Because a target's optimization goal performance in each experiment will vary somewhat each time it is run, we can only estimate mean differences in optimization goal relative to the baseline target. We express these differences as a percentage change relative to the baseline target, denoted "Δ mean %". These estimates are made to a precision that balances accuracy and cost control. We represent this precision as a 90.00% confidence interval denoted "Δ mean % CI": there is a 90.00% chance that the true value of "Δ mean %" is in that interval. We decide whether a change in performance is a "regression" -- a change worth investigating further -- if both of the following two criteria are true:
The table below, if present, lists those experiments that have experienced a statistically significant change in mean optimization goal performance between baseline and comparison SHAs with 90.00% confidence OR have been detected as newly erratic. Negative values of "Δ mean %" mean that baseline is faster, whereas positive values of "Δ mean %" mean that comparison is faster. Results that do not exhibit more than a ±5.00% change in their mean optimization goal are discarded. An experiment is erratic if its coefficient of variation is greater than 0.1. The abbreviated table will be omitted if no interesting change is observed. No interesting changes in experiment optimization goals with confidence ≥ 90.00% and |Δ mean %| ≥ 5.00%. Fine details of change detection per experiment.
|
neuronull
left a comment
neuronull
left a comment
There was a problem hiding this comment.
Thanks for the contribution @nabokihms !
Didn't quite review all of it but wanted to post what I have as a start.
| let device_version = if let Some(device_version) = self.cef.device_version.clone() { | ||
| device_version | ||
| } else { | ||
| String::from("0") // Major version. TODO(nabokihms): find a way to get the actual vector version. |
There was a problem hiding this comment.
will want to remove calling out specific GH users to the TODO comments, if any of them are intended to be merged in with the changes.
As it is they are triggering the CI spell checker.
There was a problem hiding this comment.
Removed my name from all TODO
| return Err(format!( | ||
| "severity must be a number from 0 to 10: actual {}", | ||
| severity | ||
| ) | ||
| .into()); |
There was a problem hiding this comment.
The returning of errors in this encoder could be more cleanly handled with snafu , see lib/codecs/src/encoding/format/gelf.rs for an example.
There was a problem hiding this comment.
Now all errors are handled with snafu, thanks. Fixed here.
| /// Device event identity. | ||
| #[derive(Debug, Clone)] | ||
| pub struct DeviceSettings { |
There was a problem hiding this comment.
Just struck me as odd that the doc comment "event identity" doesn't match "settings" .
Is there one or the other that makes more sense to call it and sync the doc comment to that?
There was a problem hiding this comment.
Fixed description
| let device_vendor = if let Some(device_vendor) = self.cef.device_vendor.clone() { | ||
| escape_header(device_vendor) | ||
| } else { | ||
| String::from("Datadog") | ||
| }; |
There was a problem hiding this comment.
For these, you could pass a reference in instead of cloning, see comment below for the change to the helper function.
| let device_vendor = if let Some(device_vendor) = self.cef.device_vendor.clone() { | |
| escape_header(device_vendor) | |
| } else { | |
| String::from("Datadog") | |
| }; | |
| let device_vendor = if let Some(device_vendor) = self.cef.device_vendor.as_ref() { | |
| escape_header(device_vendor) | |
| } else { | |
| String::from("Datadog") | |
| }; |
|
@neuronull thanks for reviewing! I'm on PTO this week. All the suggestions will be answered or fixed next week. |
jszwedko
added
the
meta: awaiting author
|
@nabokihms any update on this PR? :) |
|
@nabokihms are there any updates on this PR? If it's possible to finish and merge it would be very cool! The feature is really needed. |
jhgilbert
left a comment
jhgilbert
left a comment
There was a problem hiding this comment.
Approved with suggestions for style and grammar, I just flagged the first instance of everything since I know these are generated. Thank you!
website/cue/reference/components/sinks/base/aws_cloudwatch_logs.cue
Outdated
Show resolved
Hide resolved
|
Thanks, folks. I have rebased patch for this codec so I would like to continue working on merging this. https://github.com/deckhouse/deckhouse/blob/main/modules/460-log-shipper/images/vector/patches/cef-encoder.patch |
nabokihms
force-pushed
the
cef-encoder
branch
from
0446f38 to
d7121fe
Compare
github-actions
bot
added
the
domain: sinks
|
@jszwedko @neuronull I updated the PR and applied fixes according to comments and currently waiting for another round of review 🙏 |
Hi @nabokihms, thank you! I will review this PR. It is a big one, so please bear with me while I go through the code :) |
Head branch was pushed to by a user without write access
nabokihms
force-pushed
the
cef-encoder
branch
from
2a970c3 to
339732d
Compare
nabokihms
force-pushed
the
cef-encoder
branch
from
52341f0 to
90de69b
Compare
|
@pront I tried to fix the error but probably made it worth... Could you please guide me what is the issue? It seems like it is not in my code. |
Sorry, this was broken on master. You can ignore it. |
|
@nabokihms can you apply this deckhouse#447? Or you can manually do:
|
|
Sorry for the friction here, but this conflicts with the spell checker fix on master. Please revert the changes to:
You can just |
Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
nabokihms
force-pushed
the
cef-encoder
branch
from
b3d0b8e to
e96400e
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
test-misc / test-misc failed, but it seems like it was not affected by the PR. Just a flake. |
|
Thanks to all who worked on this PR. |
🎉 Thank you @nabokihms! |
7 participants
Connected to #17332
Implemented according to the guide.