From 9495848c965fb5a2beb888ff30036a894cbf69e0 Mon Sep 17 00:00:00 2001 From: Stephen Wakely Date: Thu, 29 Jun 2023 13:37:26 +0100 Subject: [PATCH 1/5] Added sink review checklist Signed-off-by: Stephen Wakely --- docs/reviews/SINK_CHECKLIST.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 docs/reviews/SINK_CHECKLIST.md diff --git a/docs/reviews/SINK_CHECKLIST.md b/docs/reviews/SINK_CHECKLIST.md new file mode 100644 index 0000000000000..27bf11fe0184b --- /dev/null +++ b/docs/reviews/SINK_CHECKLIST.md @@ -0,0 +1,29 @@ +This is a checklist to use whilst reviewing code for Vector's sinks. + +## Logic + +- [ ] Does it work? Do you understand what it is supposed to be doing? +- [ ] Does the retry logic make sense? +- [ ] Are they emitting the correct metrics? +- [ ] Are the tests testing that they are are emitting the correct metrics? +- [ ] Are there integration tests? + +## Code structure + +- [ ] Is it using the sink prelude? +- [ ] Sensible feature flags? +- [ ] Is the code modularised into `mod.rs`, `config.rs`, `sink.rs`, `request_builder.rs`, `service.rs` + +## Documentation + +- [ ] Look at the doc preview on Netlify. Does it look good? +- [ ] Is there a `cue` file linking to `base`? +- [ ] Is there a markdown file under `/website/content/en/docs/reference/configuration/sinks/`? +- [ ] Are module comments included in `mod.rs` linking to any relevant areas in the external services documentation? + +## Configuration + +- [ ] Are TLS settings configurable? +- [ ] Are the Request settings configurable? +- [ ] Proxy settings? +- [ ] Batch settings? From fdf7fc5ae81aa6ac3169d9063b7478824d1f6cbe Mon Sep 17 00:00:00 2001 From: Stephen Wakely Date: Thu, 29 Jun 2023 15:37:17 +0100 Subject: [PATCH 2/5] Added style guidelines Signed-off-by: Stephen Wakely --- docs/reviews/SINK_CHECKLIST.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/reviews/SINK_CHECKLIST.md b/docs/reviews/SINK_CHECKLIST.md index 27bf11fe0184b..a3da070354a98 100644 --- a/docs/reviews/SINK_CHECKLIST.md +++ b/docs/reviews/SINK_CHECKLIST.md @@ -13,6 +13,7 @@ This is a checklist to use whilst reviewing code for Vector's sinks. - [ ] Is it using the sink prelude? - [ ] Sensible feature flags? - [ ] Is the code modularised into `mod.rs`, `config.rs`, `sink.rs`, `request_builder.rs`, `service.rs` +- [ ] Does the code follow our [style guidelines]. ## Documentation @@ -27,3 +28,5 @@ This is a checklist to use whilst reviewing code for Vector's sinks. - [ ] Are the Request settings configurable? - [ ] Proxy settings? - [ ] Batch settings? + +[style guidelines]: https://github.com/vectordotdev/vector/blob/master/STYLE.md \ No newline at end of file From ff685ba3e4198a9d670d7b170da5679a1d25514a Mon Sep 17 00:00:00 2001 From: Stephen Wakely Date: Thu, 29 Jun 2023 15:38:06 +0100 Subject: [PATCH 3/5] Spelling Signed-off-by: Stephen Wakely --- docs/reviews/SINK_CHECKLIST.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/reviews/SINK_CHECKLIST.md b/docs/reviews/SINK_CHECKLIST.md index a3da070354a98..2c91854bcfb9b 100644 --- a/docs/reviews/SINK_CHECKLIST.md +++ b/docs/reviews/SINK_CHECKLIST.md @@ -5,14 +5,14 @@ This is a checklist to use whilst reviewing code for Vector's sinks. - [ ] Does it work? Do you understand what it is supposed to be doing? - [ ] Does the retry logic make sense? - [ ] Are they emitting the correct metrics? -- [ ] Are the tests testing that they are are emitting the correct metrics? +- [ ] Are the tests testing that they are emitting the correct metrics? - [ ] Are there integration tests? ## Code structure - [ ] Is it using the sink prelude? - [ ] Sensible feature flags? -- [ ] Is the code modularised into `mod.rs`, `config.rs`, `sink.rs`, `request_builder.rs`, `service.rs` +- [ ] Is the code modularized into `mod.rs`, `config.rs`, `sink.rs`, `request_builder.rs`, `service.rs` - [ ] Does the code follow our [style guidelines]. ## Documentation From f2c5860eacf7f5215c1e4ee2897a32253b5721cd Mon Sep 17 00:00:00 2001 From: Stephen Wakely Date: Mon, 3 Jul 2023 13:19:45 +0100 Subject: [PATCH 4/5] Addressed feedback Signed-off-by: Stephen Wakely --- docs/REVIEWING.md | 36 ++++++++++++++++++++++++++++++++++ docs/reviews/SINK_CHECKLIST.md | 32 ------------------------------ 2 files changed, 36 insertions(+), 32 deletions(-) delete mode 100644 docs/reviews/SINK_CHECKLIST.md diff --git a/docs/REVIEWING.md b/docs/REVIEWING.md index f8356f7a2f271..c006a5f3f5ce6 100644 --- a/docs/REVIEWING.md +++ b/docs/REVIEWING.md @@ -36,6 +36,40 @@ following items should also be checked: - [ ] Does it comply with [component spec](specs/component.md)? - [ ] Does it comply with the [instrumentation spec](specs/instrumentation.md)? + +### Sink review checklist + +This checklist is specific for Vector's sink code. + +## Logic + +- [ ] Does it work? Do you understand what it is supposed to be doing? +- [ ] Does the retry logic make sense? +- [ ] Are the tests testing that the sink is emitting the correct metrics? +- [ ] Are there integration tests? + +## Code structure + +- [ ] Is it using the sink prelude? +- [ ] Is it gated by sensible feature flags? +- [ ] Is the code modularized into `mod.rs`, `config.rs`, `sink.rs`, `request_builder.rs`, `service.rs` +- [ ] Does the code follow our [style guidelines]. + +## Documentation + +- [ ] Look at the doc preview on Netlify. Does it look good? +- [ ] Is there a `cue` file linking to `base`? +- [ ] Is there a markdown file under `/website/content/en/docs/reference/configuration/sinks/`? +- [ ] Are module comments included in `mod.rs` linking to any relevant areas in the external services documentation? + +## Configuration + +- [ ] Are TLS settings configurable? +- [ ] Are the Request settings configurable? +- [ ] Should it have proxy settings? If so, are they in place? +- [ ] Does it need batch settings? If so, are they used? + + ## Expectations We endeavour to review all PRs within 2 working days (Monday to Friday) of submission. @@ -131,3 +165,5 @@ your best judgment, some code requires more testing than others depending on its importance. For integrations, consider whether the code could be integration tested. + +[style guidelines]: https://github.com/vectordotdev/vector/blob/master/STYLE.md diff --git a/docs/reviews/SINK_CHECKLIST.md b/docs/reviews/SINK_CHECKLIST.md deleted file mode 100644 index 2c91854bcfb9b..0000000000000 --- a/docs/reviews/SINK_CHECKLIST.md +++ /dev/null @@ -1,32 +0,0 @@ -This is a checklist to use whilst reviewing code for Vector's sinks. - -## Logic - -- [ ] Does it work? Do you understand what it is supposed to be doing? -- [ ] Does the retry logic make sense? -- [ ] Are they emitting the correct metrics? -- [ ] Are the tests testing that they are emitting the correct metrics? -- [ ] Are there integration tests? - -## Code structure - -- [ ] Is it using the sink prelude? -- [ ] Sensible feature flags? -- [ ] Is the code modularized into `mod.rs`, `config.rs`, `sink.rs`, `request_builder.rs`, `service.rs` -- [ ] Does the code follow our [style guidelines]. - -## Documentation - -- [ ] Look at the doc preview on Netlify. Does it look good? -- [ ] Is there a `cue` file linking to `base`? -- [ ] Is there a markdown file under `/website/content/en/docs/reference/configuration/sinks/`? -- [ ] Are module comments included in `mod.rs` linking to any relevant areas in the external services documentation? - -## Configuration - -- [ ] Are TLS settings configurable? -- [ ] Are the Request settings configurable? -- [ ] Proxy settings? -- [ ] Batch settings? - -[style guidelines]: https://github.com/vectordotdev/vector/blob/master/STYLE.md \ No newline at end of file From c5618b2a4a05b051b52d4bd6eccf134fe2174cc7 Mon Sep 17 00:00:00 2001 From: Stephen Wakely Date: Tue, 11 Jul 2023 17:58:36 +0100 Subject: [PATCH 5/5] Address feedback from Kyle Signed-off-by: Stephen Wakely --- docs/REVIEWING.md | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/docs/REVIEWING.md b/docs/REVIEWING.md index c006a5f3f5ce6..26dfd8943d896 100644 --- a/docs/REVIEWING.md +++ b/docs/REVIEWING.md @@ -37,32 +37,34 @@ following items should also be checked: - [ ] Does it comply with the [instrumentation spec](specs/instrumentation.md)? -### Sink review checklist +### Checklist - new sink This checklist is specific for Vector's sink code. -## Logic +#### Logic - [ ] Does it work? Do you understand what it is supposed to be doing? - [ ] Does the retry logic make sense? - [ ] Are the tests testing that the sink is emitting the correct metrics? - [ ] Are there integration tests? -## Code structure +#### Code structure -- [ ] Is it using the sink prelude? +- [ ] Is it using the sink prelude (`use crate::sinks::prelude::*`)? +- [ ] Is the sink a stream based sink? + Check that the return value from `SinkConfig::build` is the return from `VectorSink::from_event_streamsink`. - [ ] Is it gated by sensible feature flags? - [ ] Is the code modularized into `mod.rs`, `config.rs`, `sink.rs`, `request_builder.rs`, `service.rs` - [ ] Does the code follow our [style guidelines]. -## Documentation +#### Documentation -- [ ] Look at the doc preview on Netlify. Does it look good? +- [ ] Look at the doc preview on Netlify. Does it look good? - [ ] Is there a `cue` file linking to `base`? - [ ] Is there a markdown file under `/website/content/en/docs/reference/configuration/sinks/`? - [ ] Are module comments included in `mod.rs` linking to any relevant areas in the external services documentation? -## Configuration +#### Configuration - [ ] Are TLS settings configurable? - [ ] Are the Request settings configurable?