feat: add metrics to core-utils #503

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

@@ -24,6 +26,7 @@ export class BaseService<T> {
    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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add metrics to core-utils #503

feat: add metrics to core-utils #503

annieke commented Apr 20, 2021 •

edited

Loading

changeset-bot bot commented Apr 20, 2021 •

edited

Loading

annieke commented Apr 20, 2021

annieke Apr 20, 2021

gakonst Apr 20, 2021

annieke Apr 20, 2021

gakonst commented Apr 20, 2021

gakonst commented Apr 20, 2021

annieke commented Apr 20, 2021

snario Apr 20, 2021

annieke Apr 20, 2021

annieke Apr 21, 2021

snario commented Apr 21, 2021 •

edited

Loading

gakonst commented Apr 21, 2021

gakonst left a comment

maurelian commented Apr 21, 2021

feat: add metrics to core-utils #503

feat: add metrics to core-utils #503

Conversation

annieke commented Apr 20, 2021 • edited Loading

changeset-bot bot commented Apr 20, 2021 • edited Loading

🦋 Changeset detected

annieke commented Apr 20, 2021

annieke Apr 20, 2021

Choose a reason for hiding this comment

gakonst Apr 20, 2021

Choose a reason for hiding this comment

annieke Apr 20, 2021

Choose a reason for hiding this comment

gakonst commented Apr 20, 2021

gakonst commented Apr 20, 2021

annieke commented Apr 20, 2021

snario Apr 20, 2021

Choose a reason for hiding this comment

annieke Apr 20, 2021

Choose a reason for hiding this comment

annieke Apr 21, 2021

Choose a reason for hiding this comment

snario commented Apr 21, 2021 • edited Loading

gakonst commented Apr 21, 2021

gakonst left a comment

Choose a reason for hiding this comment

maurelian commented Apr 21, 2021

annieke commented Apr 20, 2021 •

edited

Loading

changeset-bot bot commented Apr 20, 2021 •

edited

Loading

snario commented Apr 21, 2021 •

edited

Loading