ACS-8766: Adjust live-ingester logging to be more descriptive in case of error #666

krdabrowski · 2024-09-12T12:59:38Z

https://hyland.atlassian.net/browse/ACS-8766

…G and in case of error

...src/test/java/org/alfresco/hxi_connector/live_ingester/logging/MaskingPatternLayoutTest.java

.../org/alfresco/hxi_connector/live_ingester/domain/ports/ingestion_engine/DeleteNodeEvent.java

…ing-to-be-more-descriptive-in-case-of-error # Conflicts: # common-test/src/main/java/org/alfresco/hxi_connector/common/test/docker/util/DockerContainers.java

tpage-alfresco

The log quality looks much better 👍

I'm not convinced by the method of masking sensitive data out of the log using a regex. Instead I think it would be better to selectively choose data to be included in the log. With the current proposal then there's a risk that the name of a field could change, e.g. secret could be renamed token by a library upgrade, and then the masking would stop working. I don't think any of the tests here would catch this problem either.

I know it's more work, but I think it would be better to create our own text renderers for the problematic fields.

…mments

krdabrowski · 2024-09-20T10:19:28Z

@tpage-alfresco I've adjusted the way of masking stuff according to our conversation on Slack. Please could you review it one more time? 🙏

...er/adapters/messaging/hx_insight/storage/connector/PreSignedUrlRequesterIntegrationTest.java

...resco/hxi_connector/live_ingester/adapters/messaging/hx_insight/HxInsightEventPublisher.java

...onnector/live_ingester/adapters/messaging/hx_insight/storage/connector/HttpFileUploader.java

...tor/live_ingester/adapters/messaging/hx_insight/storage/connector/PreSignedUrlRequester.java

...esco/hxi_connector/live_ingester/adapters/messaging/repository/LiveIngesterEventHandler.java

.../hxi_connector/live_ingester/adapters/messaging/transform/request/ATSTransformRequester.java

.../hxi_connector/live_ingester/adapters/messaging/transform/storage/SharedFileStoreClient.java

mpichura

LGTM, as discussed, you may want to call logMaskedExchangeState direclty in .process() instead of creating/calling a private method in each RouteBuilder it used.

krdabrowski · 2024-09-20T11:19:28Z

@mpichura fixed - thanks!

tpage-alfresco · 2024-09-23T13:46:33Z

...onnector/live_ingester/adapters/messaging/hx_insight/storage/connector/HttpFileUploader.java

@@ -112,7 +116,7 @@ STORAGE_LOCATION_HEADER, wrapRawToken(fileUploadRequest.storageLocation()),
                    .withHeaders(headers)
                    .withBody(fileData)
                    .request();
-            log.atDebug().log("Upload :: {} rendition of the node: {} successfully uploaded to pre-signed URL: {}", fileUploadRequest.contentType(), nodeId, fileUploadRequest.storageLocation().getPath());
+            log.info("Storage :: Rendition of type: {} for node: {} successfully uploaded to pre-signed URL: {}", fileUploadRequest.contentType(), nodeId, fileUploadRequest.storageLocation().getPath());


You should use atInfo() to avoid the PMD issue (since PMD doesn't know how long the calls to contentType(), storageLocation() and getPath() will take).

Fixed - thanks!

tpage-alfresco · 2024-09-23T13:47:03Z

...onnector/live_ingester/adapters/messaging/hx_insight/storage/connector/HttpFileUploader.java

@@ -123,7 +127,7 @@ STORAGE_LOCATION_HEADER, wrapRawToken(fileUploadRequest.storageLocation()),
            }
            catch (IOException ioe)
            {
-                log.atDebug().log("Upload :: Stream reset failed due to: {}", ioe.getMessage());
+                log.warn("Storage :: Rendition stream NOT reset properly after node %s content upload fail due to: %s".formatted(nodeId, ioe.getMessage()), ioe);


Similarly I think we want atWarn() here.

Fixed - thanks!

…ing-to-be-more-descriptive-in-case-of-error # Conflicts: # live-ingester/src/main/java/org/alfresco/hxi_connector/live_ingester/adapters/config/jackson/DeleteNodeEventSerializer.java

…ssing PR comments

tpage-alfresco

One more change I think is worth doing.

tpage-alfresco · 2024-09-23T13:56:00Z

...nnector/live_ingester/adapters/messaging/transform/response/ATSTransformResponseHandler.java

@@ -50,6 +51,8 @@
 public class ATSTransformResponseHandler extends RouteBuilder
 {
    private static final String ROUTE_ID = "transform-events-consumer";
+    private static final int EXPECTED_STATUS_CODE = 201;
+    private static final String EXPECTED_STATUS_CODE_REGEX = "[\\s\\S]*\"status\"\\s*:\\s*%s[\\s\\S]*".formatted(EXPECTED_STATUS_CODE);


I wrote a comment suggesting this, but it disappeared - thanks GitHub!

Suggested change

private static final String EXPECTED_STATUS_CODE_REGEX = "[\\s\\S]*\"status\"\\s*:\\s*%s[\\s\\S]*".formatted(EXPECTED_STATUS_CODE);

private static final String EXPECTED_STATUS_CODE_REGEX = ".*\"status\"\\s*:\\s*%s[^0-9].*".formatted(EXPECTED_STATUS_CODE);

Explanation: I think that [\\s\\S] includes all characters so is the same as . Also the status code should not be followed by another digit (and will be followed by something in a JSON response).

The [\\s\\S] is similar to ., but it also includes whitespaces, so it covers bigger set of characters to match whole sentence / paragraph (or JSON) containing expected status

I can add the [^0-9] - this is to prevent the next char from being a number, yes?

…ssing PR comments

tpage-alfresco

Great - thanks for adding the tests!

…ssing PR comments

krdabrowski added 4 commits September 12, 2024 14:58

ACS-8766: Adjust live-ingester logging to be more descriptive in DEBU…

3c792b4

…G and in case of error

ACS-8766: Adjust live-ingester logging to be more descriptive

ab68b8e

ACS-8766: Adjust live-ingester logging to be more descriptive

ef579f1

ACS-8766: Adjust live-ingester logging to be more descriptive

367f98f

github-advanced-security bot found potential problems Sep 16, 2024

View reviewed changes

...src/test/java/org/alfresco/hxi_connector/live_ingester/logging/MaskingPatternLayoutTest.java Fixed Show fixed Hide fixed

krdabrowski added 4 commits September 16, 2024 16:19

ACS-8766: Adjust live-ingester logging to be more descriptive

f0ee3c6

ACS-8766: Adjust live-ingester logging to be more descriptive

65e4b3b

ACS-8766: Adjust live-ingester logging to be more descriptive

300c6f8

ACS-8766: Adjust live-ingester logging to be more descriptive

700d7db

github-advanced-security bot found potential problems Sep 17, 2024

View reviewed changes

.../org/alfresco/hxi_connector/live_ingester/domain/ports/ingestion_engine/DeleteNodeEvent.java Fixed Show fixed Hide fixed

ACS-8766: Adjust live-ingester logging to be more descriptive

b9e4903

github-advanced-security bot found potential problems Sep 17, 2024

View reviewed changes

.../org/alfresco/hxi_connector/live_ingester/domain/ports/ingestion_engine/DeleteNodeEvent.java Fixed Show fixed Hide fixed

krdabrowski added 3 commits September 17, 2024 12:43

ACS-8766: Adjust live-ingester logging to be more descriptive - PMD

2beb46f

ACS-8766: Adjust live-ingester logging to be more descriptive - tests

915a858

Merge branch 'master' into feature/ACS-8766-Adjust-live-ingester-logg…

de0dcb3

…ing-to-be-more-descriptive-in-case-of-error # Conflicts: # common-test/src/main/java/org/alfresco/hxi_connector/common/test/docker/util/DockerContainers.java

krdabrowski requested review from tpage-alfresco, mpichura, Pawel-608 and azakrzewski-hy September 17, 2024 11:56

tpage-alfresco requested changes Sep 17, 2024

View reviewed changes

ACS-8766: Adjust live-ingester logging to be more descriptive - PR co…

5f0b08a

…mments

krdabrowski requested a review from tpage-alfresco September 20, 2024 10:19

github-advanced-security bot found potential problems Sep 20, 2024

View reviewed changes

krdabrowski added 2 commits September 20, 2024 12:39

ACS-8766: Adjust live-ingester logging to be more descriptive - PMD

bc0c88a

ACS-8766: Adjust live-ingester logging to be more descriptive - comments

11cf350

mpichura approved these changes Sep 20, 2024

View reviewed changes

tpage-alfresco reviewed Sep 23, 2024

View reviewed changes

krdabrowski added 2 commits September 23, 2024 15:48

Merge branch 'master' into feature/ACS-8766-Adjust-live-ingester-logg…

5b79ae4

…ing-to-be-more-descriptive-in-case-of-error # Conflicts: # live-ingester/src/main/java/org/alfresco/hxi_connector/live_ingester/adapters/config/jackson/DeleteNodeEventSerializer.java

ACS-8766: Adjust live-ingester logging to be more descriptive - addre…

dcde0cc

…ssing PR comments

tpage-alfresco requested changes Sep 23, 2024

View reviewed changes

krdabrowski added 3 commits September 24, 2024 10:01

ACS-8766: Adjust live-ingester logging to be more descriptive - addre…

afc182d

…ssing PR comments

ACS-8766: Adjust live-ingester logging to be more descriptive - addre…

5defe0c

…ssing PR comments

ACS-8766: Adjust live-ingester logging to be more descriptive - addre…

9269827

…ssing PR comments

tpage-alfresco approved these changes Sep 24, 2024

View reviewed changes

ACS-8766: Adjust live-ingester logging to be more descriptive - addre…

1cda8e4

…ssing PR comments

krdabrowski merged commit 601c0b9 into master Sep 24, 2024
37 checks passed

krdabrowski deleted the feature/ACS-8766-Adjust-live-ingester-logging-to-be-more-descriptive-in-case-of-error branch September 24, 2024 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ACS-8766: Adjust live-ingester logging to be more descriptive in case of error #666

ACS-8766: Adjust live-ingester logging to be more descriptive in case of error #666

krdabrowski commented Sep 12, 2024

tpage-alfresco left a comment

krdabrowski commented Sep 20, 2024 •

edited

Loading

mpichura left a comment

krdabrowski commented Sep 20, 2024

tpage-alfresco Sep 23, 2024

krdabrowski Sep 23, 2024

tpage-alfresco Sep 23, 2024

krdabrowski Sep 23, 2024

tpage-alfresco left a comment

tpage-alfresco Sep 23, 2024

tpage-alfresco Sep 23, 2024

krdabrowski Sep 24, 2024

krdabrowski Sep 24, 2024

tpage-alfresco left a comment

	private static final String EXPECTED_STATUS_CODE_REGEX = "[\\s\\S]\"status\"\\s:\\s%s[\\s\\S]".formatted(EXPECTED_STATUS_CODE);
	private static final String EXPECTED_STATUS_CODE_REGEX = ".\"status\"\\s:\\s%s[^0-9].".formatted(EXPECTED_STATUS_CODE);

ACS-8766: Adjust live-ingester logging to be more descriptive in case of error #666

ACS-8766: Adjust live-ingester logging to be more descriptive in case of error #666

Conversation

krdabrowski commented Sep 12, 2024

tpage-alfresco left a comment

Choose a reason for hiding this comment

krdabrowski commented Sep 20, 2024 • edited Loading

mpichura left a comment

Choose a reason for hiding this comment

krdabrowski commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpage-alfresco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpage-alfresco left a comment

Choose a reason for hiding this comment

krdabrowski commented Sep 20, 2024 •

edited

Loading