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

feat: Updated metrics #10885

Merged
merged 16 commits into from
Dec 20, 2024
Merged

feat: Updated metrics #10885

merged 16 commits into from
Dec 20, 2024

Conversation

PhilWindle
Copy link
Collaborator

This PR adds a number of metrics.

Copy link
Contributor

@alexghr alexghr left a comment

Choose a reason for hiding this comment

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

Looks good, I'd change the callDuration to use the high resolution timer

valueType: ValueType.INT,
});

this.blockBuilderInsertions = meter.createGauge(Metrics.SEQUENCER_BLOCK_BUILD_INSERTION_TIME, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this intended to be a histogram?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it probably should be

@@ -135,6 +139,10 @@ export class PublicProcessor implements Traceable {
}
}

const duration = timer.s();
const rate = duration > 0 ? totalGas.l2Gas / duration : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Non blocking: I think it would be cool to calculate the rate in Grafana (e.g. adding to a counter the gas used after each tx)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think that would work would it? I think I actually want histograms here.

@@ -257,6 +263,8 @@ export class NativeWorldState implements NativeWorldStateInstance {
throw new Error('Invalid response message type: ' + response.msgType + ' != ' + messageType);
}

this.instrumentation.recordRoundTrip(callDuration * 1000, messageType);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is going to give us the resolution we want. We should be using process.hrtime for this https://nodejs.org/api/process.html#processhrtimebigint

Comment on lines 69 to 73
this.requestHistogram = meter.createHistogram(Metrics.WORLD_STATE_REQUEST_TIME, {
description: 'The round trip time of world state requests',
unit: 'microseconds',
valueType: ValueType.INT,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need buckets as the ones for ms won't be applied.


this.requestHistogram = meter.createHistogram(Metrics.WORLD_STATE_REQUEST_TIME, {
description: 'The round trip time of world state requests',
unit: 'microseconds',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
unit: 'microseconds',
unit: 'us',

Comment on lines 6 to 7
provingJobBlocks: Gauge;
provingJobTransactions: Gauge;
Copy link
Contributor

Choose a reason for hiding this comment

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

These are created as histograms.


this.totalGasHistogram = meter.createHistogram(Metrics.PUBLIC_PROCESSOR_TOTAL_GAS_HISTOGRAM, {
description: 'Total gas used in block as histogram',
unit: 'gas',
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorted, thanks

@PhilWindle PhilWindle enabled auto-merge (squash) December 20, 2024 12:02
@PhilWindle PhilWindle merged commit 79ac402 into master Dec 20, 2024
49 checks passed
@PhilWindle PhilWindle deleted the pw/metrics branch December 20, 2024 15:12
TomAFrench added a commit that referenced this pull request Dec 20, 2024
* master: (119 commits)
  chore(master): Release 0.68.0 (#10834)
  feat: enable profiling with local PXE in cli-wallet (#10736)
  chore: values for sepolia deployment (#10362)
  fix: vm_full_tests.yml (#10912)
  chore: disable bb-sanitizers.yml (#10901)
  chore: Check yarn version during bootstrap (#10910)
  fix(p2p): default peer score penalties (#10896)
  feat: Updated metrics (#10885)
  feat: Reduce bundle sizes, remove polyfills (#10877)
  chore: Replace bbup.dev link (#10908)
  chore(avm): extra column information in lookups (#10905)
  git subrepo push --branch=master noir-projects/aztec-nr
  git_subrepo.sh: Fix parent in .gitrepo file. [skip ci]
  chore: replace relative paths to noir-protocol-circuits
  git subrepo push --branch=master barretenberg
  chore: disable flake in e2e_fees/private_payments (#10898)
  chore(ci): disable e2e_cheat_codes.test.ts (#10897)
  fix: increase default heartbeat (#10891)
  fix(p2p): less verbose error (#10886)
  refactor: `contact` --> `sender` in PXE API (#10861)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants