feat: (issue 2429) read gRPC hiero-correlation-id header and prefix publisher handler logs by a-saksena · Pull Request #2534 · hiero-ledger/hiero-block-node

a-saksena · 2026-04-09T17:54:05Z

Reviewer Notes

Related Issue(s)

Use keywords Fix, Fixes, Fixed, Close, Closes, Closed, Resolve, Resolves, Resolved
to connect an issue to be closed by this pull request.

codacy-production · 2026-04-09T17:58:51Z

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric Results

Complexity 0

Duplication 0

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

jsync-swirlds

The way this was added isn't good and breaks some important metrics.

...e/stream-publisher/src/main/java/org/hiero/block/node/stream/publisher/PublisherHandler.java

block-node/stream-publisher/src/main/java/module-info.java

...sher/src/test/java/org/hiero/block/node/stream/publisher/LiveStreamPublisherManagerTest.java

...ream-publisher/src/test/java/org/hiero/block/node/stream/publisher/PublisherHandlerTest.java

...eam-publisher/src/main/java/org/hiero/block/node/stream/publisher/StreamPublisherPlugin.java

Nana-EC

Good start including passing through call chain and pre-computing correlationIdPrefix, character truncations and test additions.

@jsync-swirlds covered most of the cases I'm concerned about with respect to perf and breaking log layouts for other telemetry.
Also kindly add a PR description that makes it easier for reviewers to understand what they are reviewing and the impact

...sher/src/test/java/org/hiero/block/node/stream/publisher/LiveStreamPublisherManagerTest.java

…ler logs Signed-off-by: a-saksena <anurag@swirldslabs.com>

…ublisher handler logs. Signed-off-by: a-saksena <anurag@swirldslabs.com>

Signed-off-by: a-saksena <anurag@swirldslabs.com>

…orrelation-id header and prefix publisher handler logs Signed-off-by: a-saksena <anurag@swirldslabs.com>

Signed-off-by: a-saksena <anurag@swirldslabs.com>

codecov · 2026-04-10T04:15:09Z

Codecov Report

❌ Patch coverage is 88.15789% with 9 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
.../block/node/stream/publisher/PublisherHandler.java	85.48%	9 Missing ⚠️

@@             Coverage Diff              @@
##               main    #2534      +/-   ##
============================================
+ Coverage     81.27%   81.47%   +0.19%     
- Complexity     1512     1526      +14     
============================================
  Files           139      139              
  Lines          7131     7162      +31     
  Branches        754      757       +3     
============================================
+ Hits           5796     5835      +39     
- Misses         1010     1012       +2     
+ Partials        325      315      -10

Files with missing lines	Coverage Δ	Complexity Δ
...e/stream/publisher/LiveStreamPublisherManager.java	`84.50% <100.00%> (+0.56%)`	`69.00 <0.00> (+3.00)`
.../node/stream/publisher/StreamPublisherManager.java	`100.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...k/node/stream/publisher/StreamPublisherPlugin.java	`98.73% <100.00%> (+0.20%)`	`15.00 <5.00> (+5.00)`
.../block/node/stream/publisher/PublisherHandler.java	`91.98% <85.48%> (+5.14%)`	`77.00 <11.00> (+9.00)`

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jsync-swirlds · 2026-04-10T23:39:37Z

...e/stream-publisher/src/main/java/org/hiero/block/node/stream/publisher/PublisherHandler.java

-            LOGGER.log(TRACE, ackMessage, newLastAcknowledgedBlockNumber, handlerId);
+            LOGGER.log(
+                    TRACE,
+                    "metric-end-to-end-latency-by-block-end block={0,number,#} nsTimestamp={1,number,#} handlerId={2} correlationId=[{3}]",


Here we must not include the [] characters; it breaks parsing in Loki.

jsync-swirlds · 2026-04-10T23:40:44Z

...e/stream-publisher/src/main/java/org/hiero/block/node/stream/publisher/PublisherHandler.java

-            LOGGER.log(TRACE, traceMessage, blockNumber, currentStreamingBlockHeaderReceivedTime, handlerId);
+            LOGGER.log(
+                    TRACE,
+                    "metric-end-to-end-latency-by-block-start block={0,number,#} nsTimestamp={1,number,#} handlerId={2} correlationId=[{3}]",


Same here, this is a metric value, so we must not add extra characters that cause parsing issues.

It's a gap in our documentation but basically anywhere you see the format = is an instance of logs that Loki can convert into telemetry and we can use to create adhoc telemetry.
As such we don't want to break those but you can add key-value pairs to them

jsync-swirlds · 2026-04-10T23:45:55Z

...sher/src/test/java/org/hiero/block/node/stream/publisher/LiveStreamPublisherManagerTest.java

                assertThat(responsePipeline.getClientEndStreamCalls().get()).isEqualTo(0);
            }

+            /// Verifies that a failed [PersistedNotification] (succeeded = false)


These coverage tests will probably conflict with @ata-nas latest changes; so we should be careful about what order we merge the two PRs.

jsync-swirlds · 2026-04-10T23:47:15Z

tools-and-tests/scripts/solo-e2e-test/scripts/solo-tck-sdk-run.sh


 SDK_DIR=""
 TCK_DIR=""
-TEST_FILE="src/tests/crypto-service/test-transfer-hbar-transaction.ts src/tests/crypto-service/test-account-create-transaction.ts src/tests/crypto-service/test-account-update-transaction.ts src/tests/token-service/test-token-create-transaction.ts src/tests/token-service/test-token-update-transaction.ts src/tests/topic-service/test-topic-create-transaction.ts src/tests/topic-service/test-topic-update-transaction.ts src/tests/contract-service/test-contract-delete-transaction.ts"


Why did we remove these?

Please revert

jsync-swirlds · 2026-04-10T23:47:53Z

tools-and-tests/scripts/solo-e2e-test/scripts/solo-tck-sdk-run.sh

-if [[ "${TEST_FILE}" == "all" ]]; then
-  echo "Running all TCK tests (test:serial)"
-  npm run test:serial
-else
-  echo "Running TCK tests: $TEST_FILE"
-  # shellcheck disable=SC2086
-  # Word splitting is intentional — TEST_FILE may contain multiple space-separated file paths
-  npm run test:file $TEST_FILE
-fi


Similar concern, this seems to remove the option to run "all" tests.

cc: @Nana-EC

Yeah, please revert all changes in this file for this PR.
If we want to streamline anything here let's do it separately so this PR doesn't get held up

jsync-swirlds · 2026-04-10T23:49:04Z

...ream-publisher/src/test/java/org/hiero/block/node/stream/publisher/PublisherHandlerTest.java

                        manager.getBlockMessagingFacility().getSentNewestBlockKnownToNetworkNotifications();
                assertThat(sentNotifications).hasSize(0);
            }
+


These should also be examined by @ata-nas to ensure they aren't conflicting with his recent changes.

jsync-swirlds

A couple changes needed for the metric logs.
These are log statements we use Loki to parse into metrics because they're cross-plugin telemetry data we cannot easily turn into useable metrics otherwise.

Nana-EC

Some mote items.
We should scope this to the necessary changes needed but also watch out for the value pairs in logs, those are logs that Loki can turn into telemetry that we specifcally formatted for

Nana-EC · 2026-04-11T03:14:34Z

tools-and-tests/scripts/solo-e2e-test/scripts/solo-tck-sdk-run.sh

-if [[ "${TEST_FILE}" == "all" ]]; then
-  echo "Running all TCK tests (test:serial)"
-  npm run test:serial
-else
-  echo "Running TCK tests: $TEST_FILE"
-  # shellcheck disable=SC2086
-  # Word splitting is intentional — TEST_FILE may contain multiple space-separated file paths
-  npm run test:file $TEST_FILE
-fi


Yeah, please revert all changes in this file for this PR.
If we want to streamline anything here let's do it separately so this PR doesn't get held up

Nana-EC · 2026-04-11T03:14:53Z

tools-and-tests/scripts/solo-e2e-test/scripts/solo-tck-sdk-run.sh


 SDK_DIR=""
 TCK_DIR=""
-TEST_FILE="src/tests/crypto-service/test-transfer-hbar-transaction.ts src/tests/crypto-service/test-account-create-transaction.ts src/tests/crypto-service/test-account-update-transaction.ts src/tests/token-service/test-token-create-transaction.ts src/tests/token-service/test-token-update-transaction.ts src/tests/topic-service/test-topic-create-transaction.ts src/tests/topic-service/test-topic-update-transaction.ts src/tests/contract-service/test-contract-delete-transaction.ts"


Please revert

Nana-EC · 2026-04-11T03:24:37Z

...e/stream-publisher/src/main/java/org/hiero/block/node/stream/publisher/PublisherHandler.java

-            LOGGER.log(TRACE, traceMessage, blockNumber, currentStreamingBlockHeaderReceivedTime, handlerId);
+            LOGGER.log(
+                    TRACE,
+                    "metric-end-to-end-latency-by-block-start block={0,number,#} nsTimestamp={1,number,#} handlerId={2} correlationId=[{3}]",


It's a gap in our documentation but basically anywhere you see the format = is an instance of logs that Loki can convert into telemetry and we can use to create adhoc telemetry.
As such we don't want to break those but you can add key-value pairs to them

Nana-EC · 2026-04-11T03:44:41Z

...eam-publisher/src/main/java/org/hiero/block/node/stream/publisher/StreamPublisherPlugin.java

                context.configuration().getConfigData(ServerConfig.class).maxMessageSizeBytes() - 16384;

+        final String rawCorrelationId = options.metadata().getOrDefault("hiero-correlation-id", "");
+        if (rawCorrelationId.length() > MAX_CORRELATION_ID_LENGTH) {


I think this logic is repeated in truncateCorrelationId so you can get rid of it and just call the method

Nana-EC · 2026-04-11T03:47:57Z

...eam-publisher/src/main/java/org/hiero/block/node/stream/publisher/StreamPublisherPlugin.java

+        if (rawCorrelationId.length() > MAX_CORRELATION_ID_LENGTH) {
+            LOGGER.log(
+                    System.Logger.Level.WARNING,
+                    "Received hiero-correlation-id header exceeds {0} characters and will be truncated: {1}",


May also be worth logging the size that came in so it's clear how much over the limit we're observing

a-saksena requested review from a team as code owners April 9, 2026 17:54

a-saksena requested a review from AlfredoG87 April 9, 2026 17:54

a-saksena force-pushed the feat/2429-publisher-correlation-id branch from 7ca55b6 to 00245da Compare April 9, 2026 17:56

a-saksena self-assigned this Apr 9, 2026

a-saksena added this to the 0.32.0 milestone Apr 9, 2026

a-saksena force-pushed the feat/2429-publisher-correlation-id branch from 658508e to a221322 Compare April 9, 2026 18:34

a-saksena added the Block Node Issues/PR related to the Block Node. label Apr 9, 2026

a-saksena force-pushed the feat/2429-publisher-correlation-id branch 2 times, most recently from 239da61 to dce9517 Compare April 9, 2026 18:57

a-saksena changed the title ~~feat: (issue 2429) read gRPC hiero-correlation-id header and prefix publisher hand…~~ feat: (issue 2429) read gRPC hiero-correlation-id header and prefix publisher handler logs Apr 9, 2026

jsync-swirlds requested changes Apr 9, 2026

View reviewed changes