Skip to content
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

chore: added sink review checklist #17799

Merged
merged 5 commits into from
Jul 11, 2023
Merged

chore: added sink review checklist #17799

Changes from 1 commit
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
16 changes: 9 additions & 7 deletions docs/REVIEWING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?
StephenWakely marked this conversation as resolved.
Show resolved Hide resolved
- [ ] Are there integration tests?

## Code structure
#### 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?
- [ ] 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`
StephenWakely marked this conversation as resolved.
Show resolved Hide resolved
- [ ] 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?
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`?
neuronull marked this conversation as resolved.
Show resolved Hide resolved
- [ ] 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?
Expand Down