core: Improve SEP-1, SEP-10 logging #262
Conversation
|
Reference Server Preview is available here: |
1 similar comment
|
Reference Server Preview is available here: |
lijamie98
force-pushed
the
2022-05-17-enrich-logging
branch
2 times, most recently
from
646a4e3
to
b4833d3
Compare
|
Reference Server Preview is available here: |
lijamie98
force-pushed
the
2022-05-17-enrich-logging
branch
from
b4833d3
to
c6ddfcb
Compare
|
Reference Server Preview is available here: |
lijamie98
changed the title
|
Reference Server Preview is available here: |
| @@ -31,10 +33,16 @@ public Sep1Service(Sep1Config sep1Config, ResourceReader resourceReader) { | |||
| } | |||
|
|
|||
| public String getStellarToml() throws IOException, SepNotFoundException { | |||
| infoF("reading SEP1 TOML: {}", sep1Config.getStellarFile()); | |||
There was a problem hiding this comment.
I think you can remove this log because it'll be repeated below.
| infoF("reading SEP1 TOML: {}", sep1Config.getStellarFile()); |
There was a problem hiding this comment.
I felt a little debating here. One is for info logging and the other provides more details of how the TOML file is read in debug log.
In production, we will only see one entry. But in test or debug sessions, we will be able to see debug and info logs.
| infoF("Bad home_domain: {}", challengeRequest.getHomeDomain()); | ||
| throw new SepValidationException( | ||
| String.format("home_domain [%s] is not supported.", challengeRequest.getHomeDomain())); | ||
| } | ||
|
|
||
| if (sep10Config.isClientAttributionRequired()) { | ||
| if (challengeRequest.getClientDomain() == null) { | ||
| infoF("ALERT: client domain required and not provided"); | ||
| info("client_domain is required but not provided"); | ||
| throw new SepValidationException("client_domain is required"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Please allow me to discuss a concept here: I'd prefer to use error, rather than info logs when we're inside a catch, WDYT?
| @Test | ||
| fun testInfoBPII() { | ||
| val slot = slot<String>() | ||
| every { logger.info(capture(slot)) } answers {} | ||
|
|
||
| val testBeanPII = TestBeanPII() | ||
| Log.infoB("Hello", testBeanPII) | ||
| verify(exactly = 2) { logger.info(ofType(String::class)) } | ||
| assertFalse(slot.captured.contains("fieldPII")) | ||
| } |
There was a problem hiding this comment.
I've noticed the slot is not being used here... according with this link, you could use a list instead of a slot. It would look like this:
| @Test | |
| fun testInfoBPII() { | |
| val slot = slot<String>() | |
| every { logger.info(capture(slot)) } answers {} | |
| val testBeanPII = TestBeanPII() | |
| Log.infoB("Hello", testBeanPII) | |
| verify(exactly = 2) { logger.info(ofType(String::class)) } | |
| assertFalse(slot.captured.contains("fieldPII")) | |
| } | |
| @Test | |
| fun testInfoBPII() { | |
| val slotIMessages = mutableListOf<String>() | |
| every { logger.info(capture(slotIMessages)) } answers {} | |
| val testBeanPII = TestBeanPII() | |
| Log.infoB("Hello", testBeanPII) | |
| verify(exactly = 2) { logger.info(ofType(String::class)) } | |
| assertEquals("Hello", slotIMessages[0]) | |
| val wantBean = """{ | |
| "fieldNoPII": "no secret" | |
| }""".trimMargin() | |
| assertEquals(wantBean, slotIMessages[1]) | |
| } |
|
Reference Server Preview is available here: |
PR Checklist
PR Structure
otherwise).
paymentservice.stellar, orallordocif the changes are broad or impact manypackages.
Thoroughness
What
Improve the logging of SEP-1 and SEP-10
Why
N/A
Known limitations
N/A