feat: add metrics to core-utils by annieke · Pull Request #503 · ethereum-optimism/optimism

annieke · 2021-04-20T01:19:55Z

Description
We want to be collecting metrics in our TypeScript services. This PR adds a Metrics class to core-utils and uses it in base-service.

Additional context
The metrics class will be used to collect metrics in batch-submitter and data-transport-layer.

changeset-bot · 2021-04-20T01:19:58Z

🦋 Changeset detected

Latest commit: c15a77a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@eth-optimism/core-utils	Minor
@eth-optimism/data-transport-layer	Minor
@eth-optimism/batch-submitter	Patch
@eth-optimism/contracts	Patch
@eth-optimism/message-relayer	Patch
@eth-optimism/smock	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

annieke · 2021-04-20T01:22:32Z

@gakonst i'll need to use the updated core-utils package in batch-submitter and data-transport-layer, do i need to do anything else besides add the changeset for a new version of core-utils to be published? or can i just use master when it's merged?

happy to open another PR on the README to document these necessary steps!

annieke · 2021-04-20T01:26:18Z

packages/core-utils/src/common/metrics.ts

+export interface MetricsOptions {
+  prefix: string
+  labels?: Object
+}


@snario i based metrics.ts off of logger.ts and The Graph's Metrics. decided to keep it simple and not include passing in another registry as an option; lmk if you think that might be necessary for us though!

gakonst · 2021-04-20T06:30:47Z

packages/core-utils/src/common/metrics.ts

+
+export interface MetricsOptions {
+  prefix: string
+  labels?: Object


Should this have a more specific type?

prom-client makes it so that it's generic: https://github.com/siimon/prom-client/blame/master/README.md#L83-L91

i'm imagining adding environment as a label but not sure yet, can make an issue to make this type specific as we finalize labels, or remove until we start using labels - what do you think?

gakonst · 2021-04-20T06:32:18Z

@gakonst i'll need to use the updated core-utils package in batch-submitter and data-transport-layer, do i need to do anything else besides add the changeset for a new version of core-utils to be published? or can i just use master when it's merged?

No need for you to do anything, once this PR is merged, you'll see that the Version Packages PR will be updated with the changes from this PR's changeset!

gakonst · 2021-04-20T08:13:26Z

Running this locally, I get:

dtl_1              | Well, that's that. We ran into a fatal error. Here's the dump. Goodbye!
dtl_1              | (node:1) UnhandledPromiseRejectionWarning: Error: Invalid metric name
dtl_1              |     at new Metric (/opt/optimism/node_modules/prom-client/lib/metric.js:36:10)
dtl_1              |     at new Counter (/opt/optimism/node_modules/prom-client/lib/counter.js:12:1)
dtl_1              |     at module.exports (/opt/optimism/node_modules/prom-client/lib/metrics/processCpuTotal.js:16:30)
dtl_1              |     at Object.collectDefaultMetrics (/opt/optimism/node_modules/prom-client/lib/defaultMetrics.js:43:3)
dtl_1              |     at new Metrics (/opt/optimism/packages/core-utils/src/common/metrics.ts:29:5)
dtl_1              |     at new BaseService (/opt/optimism/packages/core-utils/src/base-service.ts:29:20)
dtl_1              |     at new L1DataTransportService (/opt/optimism/packages/data-transport-layer/src/services/main/service.ts:47:5)
dtl_1              |     at /opt/optimism/packages/data-transport-layer/src/services/run.ts:25:21
dtl_1              |     at Object.<anonymous> (/opt/optimism/packages/data-transport-layer/src/services/run.ts:63:3)
dtl_1              |     at Module._compile (internal/modules/cjs/loader.js:1063:30)
dtl_1              |     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
dtl_1              |     at Module.load (internal/modules/cjs/loader.js:928:32)
dtl_1              |     at Function.Module._load (internal/modules/cjs/loader.js:769:14)
dtl_1              |     at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
dtl_1              |     at internal/main/run_main_module.js:17:47

annieke · 2021-04-20T18:37:50Z

last commit fixed the dtl build error cc @snario

snario · 2021-04-20T19:17:35Z

packages/core-utils/src/base-service.ts

    this.name = name
    this.options = mergeDefaultOptions(options, optionSettings)
    this.logger = new Logger({ name })
+    this.metrics = new Metrics({ prefix: name.split(' ').join('_') })


Hm, this may be a bit annoying. Does the metrics client dislike spaces in service names? Perhaps we should refactor the BaseService such that names cannot have spaces in them so that the logs and metrics use identical names.

https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels only underscores and colons 😬 honestly might be the move for cleanliness sake - thoughts on importing this validation and enforcing it? https://github.com/siimon/prom-client/blob/96f7495d66b1a21755f745b1367d3e530668a957/lib/validation.js#L5-L11 or should our validation be looser?

just renamed everything without adding the validation since we will get the error on metrics initialization

snario · 2021-04-21T01:55:13Z

~~Latest integration job is failing, seems to be the #430?~~

gakonst · 2021-04-21T12:23:18Z

The failure was due to not renaming message relayer metric, which resulted in the message relayer not being brought up. Fixed in c15a77a

gakonst

LGTM

maurelian · 2021-04-21T15:12:17Z

I've removed myself as a core-utils CODEOWNER for the time being.

build(deps): update go-da to v0.1.0 build(deps): update go-da to v0.2.0; add ctx da: add cli flags for da config da: add get timeout to da client build(deps): bump local-celestia-devnet to v0.12.7 fix(cli): fix da rpc check fix(cli): check return err fix(da): blob data source reuse DataFromEVMTransactions da: upgrade to go-da v0.5.0 feat(op-batcher): support disable eth fallback chore(op-batcher): refactor EthFallbackDisabledFlagName da: use env auth token if set feat: enable multi frame txs and frame size setting for calldata txs chore: add log for multiframetxs setting da: try using blobdata for eth fallback da: add gas price flag (ethereum-optimism#451) da: reuse useblobs for multiframetxs (ethereum-optimism#452) Add Github CI (ethereum-optimism#472) Cleanup Github CI workflow (ethereum-optimism#478) feat: allow to toggle Celestia in op-batcher and op-node (ethereum-optimism#498) feat: add op-celestia indexer (ethereum-optimism#503) fix: init op-node client da: use celestia-node client (ethereum-optimism#504) da: fix batcher submit (ethereum-optimism#511)

build(deps): update go-da to v0.1.0 build(deps): update go-da to v0.2.0; add ctx da: add cli flags for da config da: add get timeout to da client build(deps): bump local-celestia-devnet to v0.12.7 fix(cli): fix da rpc check fix(cli): check return err fix(da): blob data source reuse DataFromEVMTransactions da: upgrade to go-da v0.5.0 feat(op-batcher): support disable eth fallback chore(op-batcher): refactor EthFallbackDisabledFlagName da: use env auth token if set feat: enable multi frame txs and frame size setting for calldata txs chore: add log for multiframetxs setting da: try using blobdata for eth fallback da: add gas price flag (ethereum-optimism#451) da: reuse useblobs for multiframetxs (ethereum-optimism#452) Add Github CI (ethereum-optimism#472) Cleanup Github CI workflow (ethereum-optimism#478) feat: allow to toggle Celestia in op-batcher and op-node (ethereum-optimism#498) feat: add op-celestia indexer (ethereum-optimism#503) fix: init op-node client

Closes #503

annieke added 3 commits April 19, 2021 18:12

add prom-client to core-utils

ff3deff

add metrics.ts and use in base-service

62870ed

chore: add changeset

ab8a17e

annieke requested review from ben-chain, maurelian and smartcontracts as code owners April 20, 2021 01:19

annieke requested a review from snario April 20, 2021 01:20

annieke commented Apr 20, 2021

View reviewed changes

gakonst reviewed Apr 20, 2021

View reviewed changes

format name to be valid metric prefix

da3d393

snario reviewed Apr 20, 2021

View reviewed changes

annieke changed the title ~~feat: add metrics to core-utils~~ feat: add metrics to core-utils and use in services Apr 21, 2021

rename dtl services to be metrics name compliant

e44b70a

annieke changed the title ~~feat: add metrics to core-utils and use in services~~ feat: add metrics to core-utils Apr 21, 2021

chore: produce new changeset

c85f312

fix(message-relayer): use prom-compliant metric name

c15a77a

annieke requested a review from K-Ho as a code owner April 21, 2021 12:22

gakonst approved these changes Apr 21, 2021

View reviewed changes

maurelian removed their request for review April 21, 2021 15:11

snario merged commit f950b71 into master Apr 21, 2021

snario deleted the feat/prom-client branch April 21, 2021 15:38

bap2pecs added a commit to babylonlabs-io/optimism that referenced this pull request Jul 31, 2024

chore: CI parallelism and caching (ethereum-optimism#503)

81ac2dc

theochap pushed a commit that referenced this pull request Dec 10, 2025

type annotations (#503)

c653800

theochap pushed a commit that referenced this pull request Jan 15, 2026

chore: rm unused deps (#503)

a2a190a

emhane pushed a commit that referenced this pull request Feb 4, 2026

chore: added rpc to codecov op-historical-proof component (#586)

98545a1

Closes #503

Conversation

annieke commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

annieke commented Apr 20, 2021

Uh oh!

annieke Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

gakonst Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

annieke Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

gakonst commented Apr 20, 2021

Uh oh!

gakonst commented Apr 20, 2021

Uh oh!

annieke commented Apr 20, 2021

Uh oh!

snario Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

annieke Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

annieke Apr 21, 2021

Choose a reason for hiding this comment

Uh oh!

snario commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gakonst commented Apr 21, 2021

Uh oh!

gakonst left a comment

Choose a reason for hiding this comment

Uh oh!

maurelian commented Apr 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

annieke commented Apr 20, 2021 •

edited

Loading

changeset-bot bot commented Apr 20, 2021 •

edited

Loading

snario commented Apr 21, 2021 •

edited

Loading