FINERACT-1656: Correlation ID propagation and configuration by IOhacker · Pull Request #2389 · apache/fineract

IOhacker · 2022-06-29T08:59:29Z

Description

The goal of this story is to provide a way for external systems to pass a correlation ID as a request header to Fineract APIs so that a full trace of request path can be looked at later on in a log aggregator (Kibana/Splunk/etc).

Acceptance criteria:

The correlation ID is taken from an HTTP request header
The HTTP request header’s name is configurable via a Spring property and an environment variable
The default HTTP request header’s name is X-Correlation-ID
If a correlation ID is provided, it’s inserted into the MDC context
If a correlation ID is present in the MDC context, it’s included in the log pattern
The correlation ID propagation can be enabled and disabled via a Spring property and an environment variable
Note:

The 2 properties could be called:

fineract.logging.http.correlation-id.enabled=true/false
fineract.logging.http.correlation-id.header-name=something

Jira:
https://issues.apache.org/jira/browse/FINERACT-1656

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

Write the commit message as per https://github.com/apache/fineract/#pull-requests
Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
Create/update unit or integration tests for verifying the changes made.
Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

galovics

Couple of changes needed but overall it's getting there.

@IOhacker please also add new unit test cases to verify the filter functionality.

docker-compose.yml

...r/src/main/java/org/apache/fineract/infrastructure/core/filters/CorrelationHeaderFilter.java

...ovider/src/main/java/org/apache/fineract/infrastructure/core/filters/RequestCorrelation.java

...r/src/main/java/org/apache/fineract/infrastructure/core/filters/CorrelationHeaderFilter.java

fineract-provider/src/main/resources/logback-spring.xml

fineract-provider/src/main/resources/application.properties

fineract-provider/src/test/resources/application-test.properties

fineract-war/setenv.sh

IOhacker · 2022-06-30T04:54:34Z

@galovics I am working on the Tests, ASAP I will send them

...r/src/main/java/org/apache/fineract/infrastructure/core/filters/CorrelationHeaderFilter.java

galovics · 2022-07-01T06:55:27Z

...r/src/main/java/org/apache/fineract/infrastructure/core/filters/CorrelationHeaderFilter.java

+
+    private String CORRELATION_ID_HEADER;
+
+    @Autowired


Why not using the FineractProperties here? That way you can get rid of the explicit constructor and switch it over to Lombok generating the constructor plus you are not hardcoding the environment key.

@IOhacker do you mind sharing how you tried the FineractProperties and where you got stuck with it? Thanks.

galovics · 2022-07-01T06:55:57Z

...r/src/main/java/org/apache/fineract/infrastructure/core/filters/CorrelationHeaderFilter.java

+            final HttpServletRequest httpServletRequest = (HttpServletRequest) request;            
+            currentCorrId = httpServletRequest.getHeader( CORRELATION_ID_HEADER);
+            log.debug("Found correlationId in Header : {}", currentCorrId.replaceAll("[\r\n]","") );            
+            MDC.put("correlationId", currentCorrId);            


Now, "correlationId" could be a constant to get rid of the "magic string" approach.

galovics · 2022-07-01T06:56:38Z

docker-compose-postgresql.yml

      - FINERACT_DEFAULT_TENANTDB_DESCRIPTION=Default Demo Tenant
+      - FINERACT_LOGGING_HTTP_CORRELATION_ID_ENABLED=false
+      - FINERACT_LOGGING_HTTP_CORRELATION_ID_HEADER_NAME=X-Correlation-ID
+      - CONSOLE_LOG_PATTERN=%d{yyyy-MM-dd HH:mm:ss.SSS} %thread [%X{correlationId}] [%-5level] %class{0} - %msg%n


Why don't we override the default bundled log pattern in Fineract within the application.properties?

Working on that

Now the pattern is in the application.properties

galovics · 2022-07-01T06:57:37Z

...-provider/src/test/java/org/apache/fineract/infrastructure/core/filters/CorrelationMock.java

+
+    }
+
+    public static FineractProperties.FineractCorrelationProperties createCorrelationProps(boolean enabled, String headerName) {


Let's move this into the test class. No need to be present as a separate class. At least for now.

galovics · 2022-07-01T07:01:32Z

...src/test/java/org/apache/fineract/infrastructure/core/filters/MutableHttpServletRequest.java

+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+
+final class MutableHttpServletRequest extends HttpServletRequestWrapper {


Why not simple mocking the HttpServletRequest?

galovics · 2022-07-01T07:16:33Z

...java/org/apache/fineract/infrastructure/core/filters/FineractCorrelationIdApiFilterTest.java

+        // when
+        underTest.doFilterInternal(mutableRequest, response, filterChain);
+        // then
+        verify(filterChain).doFilter(mutableRequest, response);


Nice job overall, I'm just missing one tiny piece from the tests, to verify that the MDC context has been enhanced with the correlation ID.
I know probably you were stuck there because MDC is available from a static context but what you can do is to create an MDCService in the production code which is a Spring bean and autowire it into the Filter and then use the MDCService to write into the MDC context.
This way you extracted the static MDC writing logic into a separate component that can be easily mocked out in the test.
What do you think?

I was thinking about another approach, let me share it

awasum · 2022-07-01T14:51:53Z

Hi All,

Is PERF-483 an external issue number can we try to recreate the issue on Fineract JIRA and use the issue number here so things are a bit transparent in this Open Source Project?

galovics · 2022-07-07T09:13:11Z

...r/src/main/java/org/apache/fineract/infrastructure/core/filters/CorrelationHeaderFilter.java

+
+    private String CORRELATION_ID_HEADER;
+
+    @Autowired


@IOhacker do you mind sharing how you tried the FineractProperties and where you got stuck with it? Thanks.

galovics · 2022-07-07T09:20:11Z

fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/MdcAdapter.java

+import org.slf4j.MDC;
+import org.slf4j.spi.MDCAdapter;
+
+public class MdcAdapter implements MDCAdapter {


I understand the concept but this is most definitely not going to work. On one hand the adapter is not used by anything and on the other, you're calling the MDC inside the adapter which'll end up in a recursion even if it's configured for SLF4J. The approach I suggested before would work.

galovics · 2022-07-07T09:22:22Z

...java/org/apache/fineract/infrastructure/core/filters/FineractCorrelationIdApiFilterTest.java

+@ExtendWith(MockitoExtension.class)
+@MockitoSettings(strictness = Strictness.LENIENT)
+@ContextConfiguration(classes = { Configuration.class })
+@WebMvcTest


I feel like we're overthinking this. The Filter test could be as simple as a plain unit test. We don't need WebMvc because we're not trying to verify a whole set of filters and controllers but only a single method in a class.
That'd simplify a lot of things, could you please double-check?

galovics · 2022-07-13T19:30:50Z

Closing in favor of #2427

PERF-483 Correlation ID propagation and configuration

1ed398f

galovics requested changes Jun 29, 2022

View reviewed changes

IOhacker added 10 commits June 29, 2022 13:27

PERF-483 Correlation ID propagation and configuration

5e0c6f6

PERF-483 Correlation ID propagation and configuration

f79512c

PERF-483 Correlation ID propagation and configuration

28bec10

PERF-483 Correlation ID propagation and configuration

e6a0d4c

PERF-483 Correlation ID propagation and configuration

695469b

PERF-483 Correlation ID propagation and configuration

7f9b3c4

PERF-483 Correlation ID propagation and configuration

835cdda

PERF-483 Correlation ID propagation and configuration

8a20d5a

PERF-483 Correlation ID propagation and configuration

0e54064

PERF-483 Correlation ID propagation and configuration

4d32373

PERF-483 Correlation ID propagation and configuration

41f9113

galovics requested changes Jul 1, 2022

View reviewed changes

IOhacker added 4 commits July 1, 2022 02:44

PERF-483 Correlation ID propagation and configuration

a36200d

PERF-483 Correlation ID propagation and configuration

95b7986

PERF-483 Correlation ID propagation and configuration

0d37bd5

PERF-483 Correlation ID propagation and configuration

5a5cebe

PERF-483 Correlation ID propagation and configuration

9db15fc

IOhacker changed the title ~~PERF-483 Correlation ID propagation and configuration~~ FINERACT-1656: Correlation ID propagation and configuration Jul 2, 2022

IOhacker added 8 commits July 2, 2022 19:38

FINERACT-1656 Correlation ID propagation and configuration

dea4234

FINERACT-1656 Correlation ID propagation and configuration

e1e01e9

FINERACT-1656 Correlation ID propagation and configuration

fe61204

FINERACT-1656 Correlation ID propagation and configuration

5adb7a5

FINERACT-1656 Correlation ID propagation and configuration

ca814a8

FINERACT-1656 Correlation ID propagation and configuration

d1a69a4

FINERACT-1656 Correlation ID propagation and configuration

f255906

FINERACT-1656 Correlation ID propagation and configuration

549637f

FINERACT-1656 Correlation ID propagation and configuration

6b6324f

galovics requested changes Jul 7, 2022

View reviewed changes

galovics closed this Jul 13, 2022


		}

		public static FineractProperties.FineractCorrelationProperties createCorrelationProps(boolean enabled, String headerName) {

Conversation

IOhacker commented Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

galovics left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IOhacker commented Jun 30, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awasum commented Jul 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galovics commented Jul 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

IOhacker commented Jun 29, 2022 •

edited

Loading