Skip to content

chore: added sink review checklist #17799

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

Merged
merged 5 commits into from
Jul 11, 2023
Merged

chore: added sink review checklist #17799

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9495848
Added sink review checklist
StephenWakely Jun 29, 2023
fdf7fc5
Added style guidelines
StephenWakely Jun 29, 2023
ff685ba
Spelling
StephenWakely Jun 29, 2023
f2c5860
Addressed feedback
StephenWakely Jul 3, 2023
c5618b2
Address feedback from Kyle
StephenWakely Jul 11, 2023
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
38 changes: 38 additions & 0 deletions docs/REVIEWING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,42 @@ 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)?


### Checklist - new sink

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
Copy link
Member

Choose a reason for hiding this comment

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

The checks in this section seem like they could be automated.


- [ ] 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

- [ ] Look at the doc preview on Netlify. Does it look good?
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed this sorry- Are there any specifics to ensure it looks good? Like, read through the new docs as if you were a new user trying to use that sink? Or is the message to ensure it doesn't show flag any errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just thinking mainly looking for obvious errors, eg. the cue file doesn't include the base file properly?

- [ ] Is there a `cue` file linking to `base`?
- [ ] Is there a markdown file under `/website/content/en/docs/reference/configuration/sinks/`?
Comment on lines +63 to +64
Copy link
Member

Choose a reason for hiding this comment

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

These checks seem like the could be automated too 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They could, but I wonder if it would be worth it given how infrequently new sinks get added and we would need to only check that they are correct once?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's fair 😄

- [ ] 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.
Expand Down Expand Up @@ -131,3 +167,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