2.x Redesign #437
2.x Redesign #437
Conversation
whiskeysierra
requested review from
AlexanderYastrebov and
lukasniemeier-zalando
|
Docs need to be updated. |
whiskeysierra
force-pushed
the
feature/2.x-redesign
branch
from
ed3c534 to
f83ac8f
Compare
whiskeysierra
force-pushed
the
feature/2.x-redesign
branch
from
f83ac8f to
9b14367
Compare
whiskeysierra
force-pushed
the
feature/2.x-redesign
branch
from
f5133b7 to
77a0c97
Compare
| Some of those restrictions could be mitigated with custom [`HttpLogWriter`](#writing) | ||
| implementations, but they were never ideal. | ||
|
|
||
| Starting with version 2.0 Logbook now comes with a [Strategy pattern](https://en.wikipedia.org/wiki/Strategy_pattern) |
There was a problem hiding this comment.
We may want to move the paragraphs above to the release notes of 2.0 or it's time to even provide a migration guide 😉
There was a problem hiding this comment.
This one in itself doesn't really require a migration, since it's just a new feature. But it's definitely worth having proper release notes for 2.0.
|
|
||
| @Override | ||
| public void write(final Precorrelation precorrelation, final HttpRequest request) { | ||
| sinks.forEach(throwingConsumer(sink -> sink.write(precorrelation, request))); |
There was a problem hiding this comment.
Should these be sinks.filter(Sink::isActive).forEach?
There was a problem hiding this comment.
Yes... 🤦♂️
| import static org.apiguardian.api.API.Status.STABLE; | ||
|
|
||
| @API(status = STABLE) | ||
| public interface Strategy { |
There was a problem hiding this comment.
TODO documentation? Just saying as the README teases this 😉
|
|
||
| - [`BodyOnlyIfErrorStrategy`](logbook-core/src/test/java/org/zalando/logbook/BodyOnlyIfErrorStrategy.java) | ||
| - [`ErrorResponseOnlyStrategy`](logbook-core/src/test/java/org/zalando/logbook/ErrorResponseOnlyStrategy.java) | ||
| - [`WithoutBodyStrategy`](logbook-core/src/test/java/org/zalando/logbook/WithoutBodyStrategy.java) |
There was a problem hiding this comment.
What do you think of shipping such example strategies? We know that there is a demand for such behavior and we can easily foresee that users may want to apply this to multiple services.
BodyOnlyIfStatusAtLeastStrategy(int)StatusAtLeastStrategy(int)WithoutBodyStrategy
There was a problem hiding this comment.
Yeah, probably worth it. I was hoping that the interface is so easy to use that users can just implement it within 2-5 lines.
There was a problem hiding this comment.
Agreed on the easy-to-be-used, but still gets tedious to put these 5 lines in my 25 micro-services 😉
There was a problem hiding this comment.
I created #440 as a follow-up.
I don't have an opinion how we should behave there. |
whiskeysierra
force-pushed
the
feature/2.x-redesign
branch
from
30fa1be to
4c2ca26
Compare
|
👍 |
Description
Motivation and Context
Fixes #211
Fixes #295
Fixes #296
Fixes #315
Fixes #334
Types of changes
Checklist: