Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend MetricsFacade with createSimpleTimer() factory #265

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

bradbown
Copy link
Contributor

@bradbown bradbown commented Oct 29, 2024

This PR implements issue(s) #2963
Adds:

  • Add metrics to capture the time to compress data (time spent in method GoBackedBlobCompressor canAppendBlock and appendBlock)
  • Add metrics to capture the time to perform shnarf calculation (time spent in GoBackedBlobShnarfCalculator calculateShnarf)
  • Add metrics to capture the compression ratio
  • Add metrics to capture the blob utilization ratio

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.

@bradbown bradbown temporarily deployed to docker-build-and-e2e October 29, 2024 13:52 — with GitHub Actions Inactive
@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 92.51337% with 14 lines in your changes missing coverage. Please review.

Project coverage is 68.46%. Comparing base (574e0ef) to head (21a83fd).

Files with missing lines Patch % Lines
.../consensys/zkevm/coordinator/app/L1DependentApp.kt 0.00% 6 Missing ⚠️
.../consensys/zkevm/coordinator/app/CoordinatorApp.kt 0.00% 3 Missing ⚠️
...consensys/linea/jsonrpc/JsonRpcMessageProcessor.kt 96.66% 2 Missing ⚠️
...ordination/blob/BlobCompressionProofCoordinator.kt 85.71% 1 Missing ⚠️
...linea/metrics/micrometer/DynamicTagTimerCapture.kt 75.00% 1 Missing ⚠️
...inea/metrics/micrometer/MicrometerMetricsFacade.kt 96.96% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #265      +/-   ##
============================================
+ Coverage     68.21%   68.46%   +0.25%     
- Complexity     1128     1149      +21     
============================================
  Files           319      319              
  Lines         12846    12930      +84     
  Branches       1291     1296       +5     
============================================
+ Hits           8763     8853      +90     
+ Misses         3552     3545       -7     
- Partials        531      532       +1     
Flag Coverage Δ *Carryforward flag
hardhat 98.74% <ø> (ø) Carriedforward from 574e0ef
kotlin 66.04% <92.51%> (+0.29%) ⬆️

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

Files with missing lines Coverage Δ
...zkevm/ethereum/coordination/blob/BlobCompressor.kt 91.83% <100.00%> (+2.79%) ⬆️
.../ethereum/coordination/blob/BlobShnarfCalulator.kt 76.19% <100.00%> (+1.83%) ⬆️
...m/coordination/conflation/ConflationServiceImpl.kt 82.19% <100.00%> (ø)
...sys/linea/jsonrpc/client/VertxHttpJsonRpcClient.kt 97.24% <100.00%> (+0.02%) ⬆️
...ea/jsonrpc/client/VertxHttpJsonRpcClientFactory.kt 73.42% <100.00%> (ø)
...otlin/net/consensys/linea/metrics/MetricsFacade.kt 100.00% <100.00%> (+14.28%) ⬆️
...s/linea/metrics/micrometer/AbstractTimerCapture.kt 100.00% <ø> (+19.04%) ⬆️
...sys/linea/metrics/micrometer/SimpleTimerCapture.kt 100.00% <100.00%> (+5.88%) ⬆️
...ordination/blob/BlobCompressionProofCoordinator.kt 93.54% <85.71%> (ø)
...linea/metrics/micrometer/DynamicTagTimerCapture.kt 81.81% <75.00%> (-3.04%) ⬇️
... and 4 more

... and 3 files with indirect coverage changes

@bradbown bradbown temporarily deployed to docker-build-and-e2e October 29, 2024 15:37 — with GitHub Actions Inactive
@bradbown bradbown requested a review from a team October 29, 2024 15:43
@julien-marchand julien-marchand removed the request for review from jonesho November 25, 2024 08:57
@jonesho jonesho force-pushed the metrics-facade-simple-timer-factory branch from e8c9bdc to 69ad44d Compare November 29, 2024 15:36
@jonesho jonesho temporarily deployed to docker-build-and-e2e November 29, 2024 15:36 — with GitHub Actions Inactive
@jonesho jonesho temporarily deployed to docker-build-and-e2e November 29, 2024 17:32 — with GitHub Actions Inactive
@jonesho jonesho force-pushed the metrics-facade-simple-timer-factory branch from 71d80e0 to 9858d3d Compare December 2, 2024 03:47
@jonesho jonesho temporarily deployed to docker-build-and-e2e December 2, 2024 03:47 — with GitHub Actions Inactive
@jonesho jonesho temporarily deployed to docker-build-and-e2e December 2, 2024 08:23 — with GitHub Actions Inactive
@@ -50,24 +46,18 @@ private data class RequestContext(
*/
class JsonRpcMessageProcessor(
private val requestsHandler: JsonRpcRequestHandler,
private val meterRegistry: MeterRegistry,
private val metricsFacade: MetricsFacade,
Copy link
Contributor

@jonesho jonesho Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Bradley's previous changes, JsonRpcMessageProcessor needed to be passed in both metricsFacade and metricsRegistry, and here I further reduced it to just pass in metricsFacade and added createDynamicTagTimer in MetricsFacade class

@jonesho jonesho requested a review from jpnovais December 2, 2024 09:54
@jonesho jonesho force-pushed the metrics-facade-simple-timer-factory branch from 91f7fe6 to 09c9a14 Compare December 3, 2024 04:13
@jonesho jonesho temporarily deployed to docker-build-and-e2e December 3, 2024 04:13 — with GitHub Actions Inactive
@jonesho jonesho force-pushed the metrics-facade-simple-timer-factory branch from 09c9a14 to 4f727a4 Compare December 3, 2024 11:01
@jonesho jonesho temporarily deployed to docker-build-and-e2e December 3, 2024 11:02 — with GitHub Actions Inactive
@jonesho jonesho requested review from jpnovais and removed request for jpnovais December 4, 2024 18:10
@jonesho jonesho force-pushed the metrics-facade-simple-timer-factory branch from 4f727a4 to 7ee6316 Compare December 5, 2024 06:27
@jonesho jonesho temporarily deployed to docker-build-and-e2e December 5, 2024 06:28 — with GitHub Actions Inactive
@jpnovais
Copy link
Collaborator

jpnovais commented Dec 5, 2024

I don't have bandwidth to review this pr atm. But please don't merge for now as is believe it will introduce breaking changes to the metrics.

@jonesho jonesho force-pushed the metrics-facade-simple-timer-factory branch from 7ee6316 to 63ca9c5 Compare December 6, 2024 08:28
@jonesho jonesho temporarily deployed to docker-build-and-e2e January 6, 2025 07:42 — with GitHub Actions Inactive
@jonesho jonesho force-pushed the metrics-facade-simple-timer-factory branch from 46a25f7 to 3039323 Compare January 7, 2025 08:06
@jonesho jonesho requested review from jonesho and removed request for jpnovais January 7, 2025 08:06
@jonesho jonesho temporarily deployed to docker-build-and-e2e January 7, 2025 08:07 — with GitHub Actions Inactive
@jonesho
Copy link
Contributor

jonesho commented Jan 7, 2025

I don't have bandwidth to review this pr atm. But please don't merge for now as is believe it will introduce breaking changes to the metrics.

@jpnovais the changes here should have taken care of not introducing breaking changes to existing metrics

@jpnovais
Copy link
Collaborator

jpnovais commented Jan 7, 2025

@jonesho I still see a lot of diff changes in the JSON-RPC area, It's hard by looking at the code to tell if there are breaking changes.

Can you please run coordinator and share the content of /metrics A) before the changes and B) now after these changes; ?

Copy link

cla-assistant bot commented Jan 7, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ jonesho
❌ bradbown
You have signed the CLA already but the status is still pending? Let us recheck it.

@jonesho jonesho temporarily deployed to docker-build-and-e2e January 7, 2025 16:42 — with GitHub Actions Inactive
@jonesho jonesho had a problem deploying to docker-build-and-e2e January 8, 2025 07:45 — with GitHub Actions Error
@jonesho
Copy link
Contributor

jonesho commented Jan 8, 2025

@jonesho I still see a lot of diff changes in the JSON-RPC area, It's hard by looking at the code to tell if there are breaking changes.

Can you please run coordinator and share the content of /metrics A) before the changes and B) now after these changes; ?

I also attached the "before" and "after" metrics from transaction-exclusion-api as it would create the jsonrpc metrics (those won't be created from coordinator) when handling the json-rpc requests.

Coordinator metrics before the changes:
coordinator_metrics_b4.txt

Coordinator metrics after the changes:
coordinator_metrics_after.txt

Transaction-exclusion-api metrics before the changes:
tx_exclusion_api_metrics_b4.txt

Transaction-exclusion-api metrics after the changes:
tx_exclusion_api_metrics_after.txt

@jonesho jonesho force-pushed the metrics-facade-simple-timer-factory branch from 6335832 to 21a83fd Compare January 8, 2025 07:57
@jonesho jonesho temporarily deployed to docker-build-and-e2e January 8, 2025 07:58 — with GitHub Actions Inactive
.description(
"Processing of JSON-RPC message: Deserialization + Business Logic + Serialization"
)
val timerSample = Timer.start(Clock.SYSTEM)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The time sample was taking the time here right at method beginning. Now it's move elsewhere. That will influence time measurements, no?

If read the code correctly, for instance now the time doing decodeMessage(requestJsonStr)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants