[GitHub] [fineract] galovics commented on a diff in pull request #2389: PERF-483 Correlation ID propagation and configuration

[GitBox <gi...@apache.org>]

galovics commented on code in PR #2389:
URL: https://github.com/apache/fineract/pull/2389#discussion_r911672349


##########
fineract-provider/src/test/java/org/apache/fineract/infrastructure/core/filters/MutableHttpServletRequest.java:
##########
@@ -0,0 +1,66 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.core.filters;
+
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+ 
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+ 
+final class MutableHttpServletRequest extends HttpServletRequestWrapper {

Review Comment:
   Why not simple mocking the HttpServletRequest?



##########
docker-compose-postgresql.yml:
##########
@@ -94,4 +94,7 @@ services:
       - FINERACT_DEFAULT_TENANTDB_IDENTIFIER=default
       - FINERACT_DEFAULT_TENANTDB_NAME=fineract_default
       - 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

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



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/filters/CorrelationHeaderFilter.java:
##########
@@ -0,0 +1,75 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fineract.infrastructure.core.filters;
+
+import org.slf4j.MDC;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.core.env.Environment;
+import org.springframework.web.filter.OncePerRequestFilter;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+import javax.servlet.FilterChain;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import java.io.IOException;
+
+@RequiredArgsConstructor
+@Slf4j
+public class CorrelationHeaderFilter extends OncePerRequestFilter  {
+
+    private String CORRELATION_ID_HEADER;
+
+    @Autowired
+    public CorrelationHeaderFilter(Environment env) {
+        CORRELATION_ID_HEADER = env.getRequiredProperty("fineract.logging.http.correlation-id.header-name");        
+    }	
+   
+    @Override
+    protected void doFilterInternal(final HttpServletRequest request, final HttpServletResponse response, final FilterChain filterChain)
+        throws IOException, ServletException  {        
+        String currentCorrId="";
+        try {
+            
+            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);            
+            filterChain.doFilter(request, response);
+        } 
+        finally {
+            MDC.remove(currentCorrId);

Review Comment:
   You should delete the key from the MDC context, not the value.



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/filters/CorrelationHeaderFilter.java:
##########
@@ -0,0 +1,75 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fineract.infrastructure.core.filters;
+
+import org.slf4j.MDC;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.core.env.Environment;
+import org.springframework.web.filter.OncePerRequestFilter;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+import javax.servlet.FilterChain;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import java.io.IOException;
+
+@RequiredArgsConstructor
+@Slf4j
+public class CorrelationHeaderFilter extends OncePerRequestFilter  {
+
+    private String CORRELATION_ID_HEADER;
+
+    @Autowired
+    public CorrelationHeaderFilter(Environment env) {
+        CORRELATION_ID_HEADER = env.getRequiredProperty("fineract.logging.http.correlation-id.header-name");        
+    }	
+   
+    @Override
+    protected void doFilterInternal(final HttpServletRequest request, final HttpServletResponse response, final FilterChain filterChain)
+        throws IOException, ServletException  {        
+        String currentCorrId="";
+        try {
+            
+            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);            

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



##########
fineract-provider/src/test/java/org/apache/fineract/infrastructure/core/filters/CorrelationMock.java:
##########
@@ -0,0 +1,35 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.core.filters;
+
+import org.apache.fineract.infrastructure.core.config.FineractProperties;
+
+public final class CorrelationMock {
+
+    private CorrelationMock() {
+
+    }
+
+    public static FineractProperties.FineractCorrelationProperties createCorrelationProps(boolean enabled, String headerName) {

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



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/filters/CorrelationHeaderFilter.java:
##########
@@ -0,0 +1,75 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fineract.infrastructure.core.filters;
+
+import org.slf4j.MDC;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.core.env.Environment;
+import org.springframework.web.filter.OncePerRequestFilter;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+import javax.servlet.FilterChain;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import java.io.IOException;
+
+@RequiredArgsConstructor
+@Slf4j
+public class CorrelationHeaderFilter extends OncePerRequestFilter  {
+
+    private String CORRELATION_ID_HEADER;
+
+    @Autowired

Review Comment:
   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.



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/filters/CorrelationHeaderFilter.java:
##########
@@ -0,0 +1,75 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fineract.infrastructure.core.filters;
+
+import org.slf4j.MDC;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.core.env.Environment;
+import org.springframework.web.filter.OncePerRequestFilter;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+import javax.servlet.FilterChain;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import java.io.IOException;
+
+@RequiredArgsConstructor
+@Slf4j
+public class CorrelationHeaderFilter extends OncePerRequestFilter  {
+
+    private String CORRELATION_ID_HEADER;
+
+    @Autowired
+    public CorrelationHeaderFilter(Environment env) {
+        CORRELATION_ID_HEADER = env.getRequiredProperty("fineract.logging.http.correlation-id.header-name");        
+    }	
+   
+    @Override
+    protected void doFilterInternal(final HttpServletRequest request, final HttpServletResponse response, final FilterChain filterChain)
+        throws IOException, ServletException  {        
+        String currentCorrId="";
+        try {
+            
+            final HttpServletRequest httpServletRequest = (HttpServletRequest) request;            
+            currentCorrId = httpServletRequest.getHeader( CORRELATION_ID_HEADER);
+            log.debug("Found correlationId in Header : {}", currentCorrId.replaceAll("[\r\n]","") );            

Review Comment:
   You could use LogParameterEscapeUtil here for the log param.



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/filters/CorrelationHeaderFilter.java:
##########
@@ -0,0 +1,75 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fineract.infrastructure.core.filters;
+
+import org.slf4j.MDC;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.core.env.Environment;
+import org.springframework.web.filter.OncePerRequestFilter;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+import javax.servlet.FilterChain;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import java.io.IOException;
+
+@RequiredArgsConstructor
+@Slf4j
+public class CorrelationHeaderFilter extends OncePerRequestFilter  {
+
+    private String CORRELATION_ID_HEADER;

Review Comment:
   This doesn't conform the Java naming conventions. Should be camelCase.



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/filters/CorrelationHeaderFilter.java:
##########
@@ -0,0 +1,75 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fineract.infrastructure.core.filters;
+
+import org.slf4j.MDC;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.core.env.Environment;
+import org.springframework.web.filter.OncePerRequestFilter;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+import javax.servlet.FilterChain;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import java.io.IOException;
+
+@RequiredArgsConstructor
+@Slf4j
+public class CorrelationHeaderFilter extends OncePerRequestFilter  {
+
+    private String CORRELATION_ID_HEADER;
+
+    @Autowired
+    public CorrelationHeaderFilter(Environment env) {
+        CORRELATION_ID_HEADER = env.getRequiredProperty("fineract.logging.http.correlation-id.header-name");        
+    }	
+   
+    @Override
+    protected void doFilterInternal(final HttpServletRequest request, final HttpServletResponse response, final FilterChain filterChain)
+        throws IOException, ServletException  {        
+        String currentCorrId="";

Review Comment:
   No need for this variable here, should be local to where the header is accessed.



##########
fineract-provider/src/test/java/org/apache/fineract/infrastructure/core/filters/FineractCorrelationIdApiFilterTest.java:
##########
@@ -0,0 +1,102 @@
+package org.apache.fineract.infrastructure.core.filters;
+
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.verify;
+
+
+import java.io.IOException;
+import java.io.PrintWriter;
+import javax.servlet.FilterChain;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import org.apache.fineract.infrastructure.core.config.FineractProperties;
+import org.apache.fineract.infrastructure.core.filters.CorrelationHeaderFilter;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+import org.mockito.junit.jupiter.MockitoSettings;
+import org.mockito.quality.Strictness;
+import org.springframework.http.HttpMethod;
+
+@ExtendWith(MockitoExtension.class)
+@MockitoSettings(strictness = Strictness.LENIENT)
+class FineractCorrelationIdApiFilterTest {
+
+    @Mock
+    private FineractProperties fineractProperties;
+
+    @Mock
+    private HttpServletRequest request;
+
+    @Mock
+    private HttpServletResponse response;
+
+    @Mock
+    private FilterChain filterChain;
+
+    @Mock
+    private PrintWriter outputWriter;
+
+    @InjectMocks
+    private CorrelationHeaderFilter underTest;
+
+    @BeforeEach
+    void setUp() throws IOException {
+        given(response.getWriter()).willReturn(outputWriter);
+    }
+
+    @Test
+    void testDoFilterInternal_ShouldLetReadApisThrough_WhenFineractIsInCorrelationAndIsGetApi() throws ServletException, IOException {
+        // given
+        FineractProperties.FineractCorrelationProperties correlationProperties = CorrelationMock.createCorrelationProps(true, "X-Correlation-ID");
+        given(fineractProperties.getCorrelation()).willReturn(correlationProperties);        
+        given(request.getMethod()).willReturn(HttpMethod.GET.name());
+        given(request.getPathInfo()).willReturn("/loans");
+        MutableHttpServletRequest mutableRequest = new MutableHttpServletRequest(request);        
+        mutableRequest.putHeader(correlationProperties.getHeaderName(), "123456ABCDEF");
+        // when
+        underTest.doFilterInternal(mutableRequest, response, filterChain);
+        // then
+        verify(filterChain).doFilter(mutableRequest, response);

Review Comment:
   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?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org