Graylog2 / Graylog2/graylog-plugin-integrations

AWS Kinesis Code Improvement

Open
#337 0 comments 0 reactions 0 assignees View on GitHub
aws improvement
Dominant language
Java
Stars
17
Forks
14
PR merge metrics
No merged PRs in 30d

Description

# Description
@alex-konn Did a review of the AWS Kinesis code, and provided some great suggestions for improvement of the code. I am capturing these in an issue so they can be addressed outside of PR #279.

## Metrics

- Maybe it makes sense to provide a predefined dashboard with graylog's
own kinesis metrics to see, how far "our" workers are behind with
processing the stream
- https://docs.aws.amazon.com/streams/latest/dev/monitoring-with-kcl.html
- Unit tests would be helpful, would require mocking out the assume role
call

## Comments
- I would get rid of most of the comments as the code is pretty concise
and clear, no need to restate what it does

- Keep comments explaining WHY some things are done though, as the assume
role stuff is not very intuitive
- In general i find that most comments do more harm than good by
bloating the amount of text one has to grok to understand what a class
does and falling out of sync with the code when people change code but
forget to update comments
- [ ] Remove comments https://github.com/Graylog2/graylog-plugin-integrations/pull/409

## AWSRequestImpl
- [ ] Get rid of `AWSRequest` interface and just use this as base class
=> no repeated field definitions while retaining type safety

## CloudWatchService
- I wouldn't use javax exceptions like `BadRequestException` here. it
couples the services to jersey and being used in a http service.
but changing that might be out of scope for this pr. also i've seen
this in many places in graylog-server already. so it might be something
like a convention

- [ ] Remove `BadRequestException` in CloudWatch Service

- the "correct" way for me would be throw a custom `RuntimeException`
and either catch and throw `BadRequestException` in the resource or use
a jersey exception mapper to map the exception to an http response
- [ ] Improve how exceptions are thrown in CloudWatch Service

- Another general comment: i find such long methods hard to understand.
it helps to extract short private methods with meaningful names.
that's also a very useful technique to discover parts of related code
that can later be extracted to their own classes with a single
responsibility.
e.g., `addSubscriptionFilter`
- [ ] extract short private methods with meaningful names

## KinesisServiceTest
- Needs guice setup for ignored test to run (and fail)
GuiceInjectorHolder.createInjector(Collections.emptyList());
- [ ] Include Guice set up for unit tests

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.