feat: centralize configuration logging #501
Conversation
mattp-swirldslabs
added
Feature Enhancement
|
@mattp-swirldslabs, this should be part of Milestone |
There was a problem hiding this comment.
Good job @mattp-swirldslabs! the logging looks really good!
I do think however that some changes are due in the sanity test and in the way we handle config classes we do not own. Also some optional nits and cleanup suggested.
server/src/main/java/com/hedera/block/server/config/ServerMappedConfigSourceInitializer.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/config/logging/ConfigurationLoggingImpl.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/config/logging/Loggable.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/consumer/ConsumerConfig.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/mediator/MediatorConfig.java
Outdated
Show resolved
Hide resolved
...er/src/test/java/com/hedera/block/server/config/ServerMappedConfigSourceInitializerTest.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/config/logging/ConfigurationLoggingImpl.java
Outdated
Show resolved
Hide resolved
server/src/test/java/com/hedera/block/server/mediator/MediatorConfigTest.java
Show resolved
Hide resolved
server/src/test/java/com/hedera/block/server/verification/VerificationInjectionModuleTest.java
Show resolved
Hide resolved
...t/java/com/hedera/block/server/verification/session/BlockVerificationSessionFactoryTest.java
Show resolved
Hide resolved
Signed-off-by: Matt Peterson <[email protected]>
mattp-swirldslabs
force-pushed
the
00471-centralize-config-logging
branch
from
8a550cf to
e167599
Compare
Signed-off-by: Matt Peterson <[email protected]>
Signed-off-by: Matt Peterson <[email protected]>
Signed-off-by: Matt Peterson <[email protected]>
Signed-off-by: Matt Peterson <[email protected]>
Signed-off-by: Matt Peterson <[email protected]>
Signed-off-by: Matt Peterson <[email protected]>
server/src/main/java/com/hedera/block/server/config/logging/ConfigurationLoggingImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Matt Peterson <[email protected]>
Signed-off-by: Matt Peterson <[email protected]>
…communicate that the value is missing Signed-off-by: Matt Peterson <[email protected]>
Signed-off-by: Matt Peterson <[email protected]>
There was a problem hiding this comment.
LGTM.
@mattp-swirldslabs @ata-nas @jsync-swirlds let's try to capture nits, improvements and suggestions on a follow-up ticket, and move forward with this, we cannot afford to spend any more time on some of the nits am reading here. This is way better than what we used to have (no logging or visibility of configuration loaded by the system).
My suggestions is to merge as currently is this PR.
I would agree to that, @mattp-swirldslabs please kindly capture/acknowledge nits/considerations which came up above and for me the conversations would be considered resolved (which are not even final, could be or not be implemented in the long run, mind you there are already planned changes to the configuration with the query endpoint) and we can proceed. P.S. @AlfredoG87 I agree on the time sensitivity, I am not trying to block. just some considerations come and I want to acknowledge them. My ask is not to resolve everything now, just to capture what comes to mind. |
Description:
ConfigurationLoggingImplclass@Loggableannotation to allow developers to indicate a field can be logged normally@Loggableare safe by default. The property name will be displayed but the value will always be the same "*****" e.g.my.property=*****enabledwith an enumRelated issue(s):
Fixes #471
Fixes #474
Notes for reviewer:
Checklist