-
Notifications
You must be signed in to change notification settings - Fork 3
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.
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
feat: centralize configuration logging #501
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work.
@mattp-swirldslabs, this should be part of Milestone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
Description:
ConfigurationLoggingImpl
class@Loggable
annotation to allow developers to indicate a field can be logged normally@Loggable
are safe by default. The property name will be displayed but the value will always be the same "*****" e.g.my.property=*****
enabled
with an enumRelated issue(s):
Fixes #471
Fixes #474
Notes for reviewer:
Checklist