feat: add metrics in GlobalBlobAwareConflationCalculator and move bri… #654

jonesho · 2025-02-05T08:10:49Z

…dge logs and state root hash retrieval from execution prover client to coordinator

This PR implements issue(s) #604

Checklist

I wrote new tests for my new core changes.
I have successfully ran tests, style checker and build against my new changes locally.
I have informed the team of any breaking changes if there are any.

codecov-commenter · 2025-02-05T08:25:54Z

Codecov Report

Attention: Patch coverage is 83.33333% with 26 lines in your changes missing coverage. Please review.

Project coverage is 68.30%. Comparing base (3c4f4c2) to head (728d552).

Files with missing lines	Patch %	Lines
...in/kotlin/net/consensys/zkevm/domain/Conflation.kt	25.00%	7 Missing and 2 partials ⚠️
.../conflation/GlobalBlobAwareConflationCalculator.kt	90.12%	4 Missing and 4 partials ⚠️
.../consensys/zkevm/coordinator/app/L1DependentApp.kt	0.00%	5 Missing ⚠️
...on/proofcreation/ZkProofCreationCoordinatorImpl.kt	93.75%	2 Missing ⚠️
...clients/prover/FileBasedExecutionProverClientV2.kt	93.75%	1 Missing ⚠️
.../coordinator/clients/prover/ProverClientFactory.kt	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #654      +/-   ##
============================================
+ Coverage     68.08%   68.30%   +0.22%     
- Complexity     1182     1198      +16     
============================================
  Files           326      326              
  Lines         13229    13325      +96     
  Branches       1338     1349      +11     
============================================
+ Hits           9007     9102      +95     
+ Misses         3667     3665       -2     
- Partials        555      558       +3

Flag	Coverage Δ		*Carryforward flag
hardhat	`98.52% <ø> (ø)`		Carriedforward from 3c4f4c2
kotlin	`65.99% <83.33%> (+0.25%)`	⬆️

*This pull request uses carry forward flags. Click here to find out more.

Files with missing lines	Coverage Δ
...tor/clients/BatchExecutionProverRequestResponse.kt	`81.81% <100.00%> (+37.37%)`	⬆️
...on/conflation/BlockToBatchSubmissionCoordinator.kt	`92.10% <100.00%> (+0.43%)`	⬆️
...conflation/ConflationCalculatorByDataCompressed.kt	`96.00% <100.00%> (ø)`
...clients/prover/FileBasedExecutionProverClientV2.kt	`48.97% <93.75%> (-10.04%)`	⬇️
.../coordinator/clients/prover/ProverClientFactory.kt	`62.50% <0.00%> (+2.15%)`	⬆️
...on/proofcreation/ZkProofCreationCoordinatorImpl.kt	`94.28% <93.75%> (+94.28%)`	⬆️
.../consensys/zkevm/coordinator/app/L1DependentApp.kt	`1.46% <0.00%> (-0.01%)`	⬇️
.../conflation/GlobalBlobAwareConflationCalculator.kt	`93.52% <90.12%> (-1.56%)`	⬇️
...in/kotlin/net/consensys/zkevm/domain/Conflation.kt	`59.37% <25.00%> (-0.28%)`	⬇️

... and 4 files with indirect coverage changes

...in/kotlin/net/consensys/zkevm/coordinator/clients/prover/FileBasedExecutionProverClientV2.kt

...in/net/consensys/zkevm/ethereum/coordination/proofcreation/ZkProofCreationCoordinatorImpl.kt

.../net/consensys/zkevm/ethereum/coordination/conflation/GlobalBlobAwareConflationCalculator.kt

jpnovais · 2025-02-12T09:23:33Z

.../net/consensys/zkevm/ethereum/coordination/conflation/GlobalBlobAwareConflationCalculator.kt

+  )
+  private val gasUsedInBatchHistogram = metricsFacade.createHistogram(
+    category = LineaMetricsCategory.BATCH,
+    name = "gas.used",


Same note as the blob

.../consensys/zkevm/ethereum/coordination/aggregation/ProofAggregationCoordinatorServiceTest.kt

.../consensys/zkevm/ethereum/coordination/conflation/GlobalBlobAwareConflationCalculatorTest.kt

…dge logs and state root hash retrieval from execution prover client to coordinator

…istogram metrics

…nv vars in docker compose

jonesho · 2025-02-21T05:58:19Z

docker/compose-spec-l1-services.yml

        /usr/local/bin/eth2-testnet-genesis deneb \
-        --config /config/network-config.yml \${L1_GENESIS_TIME:+--timestamp ${L1_GENESIS_TIME} \}
+        --config /config/network-config.yml \${L1_GENESIS_TIME:+--timestamp ${L1_GENESIS_TIME:-} \}


To address the warning logs of missing env variables when running docker compose

Filter94 · 2025-02-21T11:09:23Z

.../net/consensys/zkevm/ethereum/coordination/conflation/GlobalBlobAwareConflationCalculator.kt

+        .filter { blobInterval.blocksRange.contains(it.blockNumber) }
+        .sumOf { it.gasUsed }
+      val uncompressedDataSizeInBlob = blobBlockCounters
+        .filter { blobInterval.blocksRange.contains(it.blockNumber) }


We can do the filter once here and reuse the result

Filter94 · 2025-02-21T11:17:02Z

coordinator/core/build.gradle

Side note.

implementation project(":jvm-libs:linea:metrics:micrometer")

should be replaced by
testFixturesImplementation project(":jvm-libs:linea:metrics:micrometer")

It would be nice to clean this up in this change. Our core implementation doesn't really depend on Micrometer

Filter94 · 2025-02-21T11:18:28Z

.../consensys/zkevm/ethereum/coordination/aggregation/ProofAggregationCoordinatorServiceTest.kt

+    lateinit var mockAggregationSizeInBlocksHistogram: Histogram
+    lateinit var mockAggregationSizeInBatchesHistogram: Histogram
+    lateinit var mockAggregationSizeInBlobsHistogram: Histogram
+    val mockMetricsFacade: MetricsFacade = mock() {


Isn't it easier to create a real instance instead? We have the micrometer dependency in the test fixtures anyway

Filter94 · 2025-02-21T11:23:49Z

.../consensys/zkevm/ethereum/coordination/conflation/GlobalBlobAwareConflationCalculatorTest.kt

+  private lateinit var mockMetricsFacade: MetricsFacade
+
+  // histogram metrics mocks
+  private lateinit var mockGasUsedInBlobHistogram: Histogram


I feel like this is an ideal case for a fake implementation backed by a list. Mocks instantiation is too repetitive IMO and the only advantage we're taking from the mocks is the inOrder verification. IMO a simple Histogram implementation will cover our needs much better with less repetitive and more readable code

jonesho added the Coordinator label Feb 5, 2025

jonesho self-assigned this Feb 5, 2025

jonesho marked this pull request as draft February 5, 2025 08:10

jonesho temporarily deployed to docker-build-and-e2e February 5, 2025 08:11 — with GitHub Actions Inactive

jonesho had a problem deploying to docker-build-and-e2e February 5, 2025 08:17 — with GitHub Actions Error

jonesho temporarily deployed to docker-build-and-e2e February 5, 2025 08:47 — with GitHub Actions Inactive

jonesho temporarily deployed to docker-build-and-e2e February 5, 2025 09:30 — with GitHub Actions Inactive

jonesho marked this pull request as ready for review February 5, 2025 19:11

jonesho force-pushed the 604-export-extra-metrics-for-conflation-blob-and-aggregation branch from 1412aaf to 40c25db Compare February 6, 2025 05:44

jonesho temporarily deployed to docker-build-and-e2e February 6, 2025 05:45 — with GitHub Actions Inactive

jonesho commented Feb 6, 2025

View reviewed changes

...in/kotlin/net/consensys/zkevm/coordinator/clients/prover/FileBasedExecutionProverClientV2.kt Show resolved Hide resolved

...in/net/consensys/zkevm/ethereum/coordination/proofcreation/ZkProofCreationCoordinatorImpl.kt Outdated Show resolved Hide resolved

jonesho requested a review from a team February 6, 2025 18:32

jpnovais reviewed Feb 7, 2025

View reviewed changes

...in/net/consensys/zkevm/ethereum/coordination/proofcreation/ZkProofCreationCoordinatorImpl.kt Outdated Show resolved Hide resolved

...in/net/consensys/zkevm/ethereum/coordination/proofcreation/ZkProofCreationCoordinatorImpl.kt Outdated Show resolved Hide resolved

jonesho force-pushed the 604-export-extra-metrics-for-conflation-blob-and-aggregation branch from 40c25db to 644ca9c Compare February 11, 2025 08:59

jonesho temporarily deployed to docker-build-and-e2e February 11, 2025 08:59 — with GitHub Actions Inactive

jonesho requested a review from jpnovais February 11, 2025 14:21

jonesho force-pushed the 604-export-extra-metrics-for-conflation-blob-and-aggregation branch from 644ca9c to c25c3f4 Compare February 11, 2025 17:32

jonesho temporarily deployed to docker-build-and-e2e February 11, 2025 17:33 — with GitHub Actions Inactive

jonesho temporarily deployed to docker-build-and-e2e February 11, 2025 18:15 — with GitHub Actions Inactive

jonesho temporarily deployed to docker-build-and-e2e February 11, 2025 19:24 — with GitHub Actions Inactive

jonesho temporarily deployed to docker-build-and-e2e February 12, 2025 03:47 — with GitHub Actions Inactive

jonesho requested review from jpnovais and removed request for jpnovais February 12, 2025 08:24

jonesho temporarily deployed to docker-build-and-e2e February 12, 2025 08:25 — with GitHub Actions Inactive

jpnovais reviewed Feb 12, 2025

View reviewed changes

jonesho force-pushed the 604-export-extra-metrics-for-conflation-blob-and-aggregation branch from 787148c to e67bbf9 Compare February 13, 2025 14:24

jonesho temporarily deployed to docker-build-and-e2e February 13, 2025 14:25 — with GitHub Actions Inactive

jonesho temporarily deployed to docker-build-and-e2e February 14, 2025 18:24 — with GitHub Actions Inactive

jonesho temporarily deployed to docker-build-and-e2e February 15, 2025 19:37 — with GitHub Actions Inactive

jonesho requested a review from jpnovais February 17, 2025 18:01

jonesho temporarily deployed to docker-build-and-e2e February 18, 2025 06:01 — with GitHub Actions Inactive

jonesho temporarily deployed to docker-build-and-e2e February 20, 2025 08:28 — with GitHub Actions Inactive

jonesho added 12 commits February 21, 2025 01:06

feat: add metrics in GlobalBlobAwareConflationCalculator and move bri…

06f60e3

…dge logs and state root hash retrieval from execution prover client to coordinator

fix: unit test

78d9bd8

feat: testing for docker cache pipeline

4e653a0

feat: disabled docker cache

6e649bb

feat: move encoder back to file-based prover client

3dafb90

fix: compare with hex string instead of ulong for bridgelog block number

c03a342

feat: add test case for ExecutionProofRequestDtoMapper

24e75e5

feat: revise ExecutionProofRequestDtoMapper test case

1ece72f

feat: revise metric names and add testing for the added metrics

4526571

feat: added metricscheck

b8ad147

feat: handle zero transactions in empty blocks for avg-tx-data-size h…

0661336

…istogram metrics

feat: resolve conflicts from latest main and fix warning on missing e…

b633da7

…nv vars in docker compose

jonesho force-pushed the 604-export-extra-metrics-for-conflation-blob-and-aggregation branch from 115464d to b633da7 Compare February 20, 2025 17:37

jonesho temporarily deployed to docker-build-and-e2e February 20, 2025 17:38 — with GitHub Actions Inactive

fix: unit tests

728d552

jonesho temporarily deployed to docker-build-and-e2e February 20, 2025 17:44 — with GitHub Actions Inactive

jonesho commented Feb 21, 2025

View reviewed changes

jonesho requested a review from Filter94 February 21, 2025 10:25

Filter94 reviewed Feb 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add metrics in GlobalBlobAwareConflationCalculator and move bri… #654

feat: add metrics in GlobalBlobAwareConflationCalculator and move bri… #654

jonesho commented Feb 5, 2025

codecov-commenter commented Feb 5, 2025 •

edited

Loading

jpnovais Feb 12, 2025

jonesho Feb 21, 2025

Filter94 Feb 21, 2025

Filter94 Feb 21, 2025

Filter94 Feb 21, 2025

Filter94 Feb 21, 2025

feat: add metrics in GlobalBlobAwareConflationCalculator and move bri… #654

Are you sure you want to change the base?

feat: add metrics in GlobalBlobAwareConflationCalculator and move bri… #654

Conversation

jonesho commented Feb 5, 2025

Checklist

codecov-commenter commented Feb 5, 2025 • edited Loading

Codecov Report

jpnovais Feb 12, 2025

Choose a reason for hiding this comment

jonesho Feb 21, 2025

Choose a reason for hiding this comment

Filter94 Feb 21, 2025

Choose a reason for hiding this comment

Filter94 Feb 21, 2025

Choose a reason for hiding this comment

Filter94 Feb 21, 2025

Choose a reason for hiding this comment

Filter94 Feb 21, 2025

Choose a reason for hiding this comment

codecov-commenter commented Feb 5, 2025 •

edited

Loading