133/prometheus #173

bartmacbartek · 2021-04-27T22:41:20Z

Closes #133

bartmacbartek · 2021-04-27T22:42:36Z

prometheus.yaml

+global:
+  scrape_interval: 15s
+  evaluation_interval: 15s
+
+scrape_configs:
+  - job_name: confio_relayer
+
+    scrape_interval: 5s
+    static_configs:
+      - targets: ['host.docker.internal:26660']


This is an example configuration file. Is this the right place to store it?

Good to commit it to this repo, so it is maintained.

You could put it in a samples or ops dir. We can add example systemd and other configs there as needed.
(Maybe we move it together with the registry.conf file?)

I realized that init command has registry.yaml hardcoded to main branch, so I cannot move registry under samples.

ts-relayer/src/binary/ibc-setup/commands/init.ts

Line 49 in d8a594b

'https://raw.githubusercontent.com/confio/ts-relayer/main/demo/registry.yaml'

One way to "fix" this would be to use parametrized link with version:

`https://raw.githubusercontent.com/confio/ts-relayer/tree/v${packageJson.version}/demo/registry.yaml`

But it fixes the future versions only. Everything till 0.1.3 included will break.

👉 For now, I moved prometheus.yaml under demo. 92287b8

And let's keep it in demo for now (marked deprecated), and make the main copy in another directory (final location)

I am not sure about fixing to a version. We can think about a better location, but nice to be able to roll out registry without requiring upgrades

bartmacbartek · 2021-04-27T22:45:06Z

src/binary/ibc-relayer/setup-prometheus.ts

+    return getStubbedMetrics();
+  }
+
+  client.collectDefaultMetrics({ prefix: 'confio_relayer_' });


Not sure if we want to collect these? Reference:

https://github.com/siimon/prom-client#default-metrics

https://prometheus.io/docs/instrumenting/writing_clientlibs/#process-metrics

bartmacbartek · 2021-04-27T22:51:01Z

src/binary/ibc-relayer/commands/start.ts

@@ -262,5 +294,7 @@ async function relayerLoop(link: Link, options: Options, logger: Logger) {
    logger.info(`Sleeping ${options.poll} seconds...`);
    await sleep(options.poll * 1000);
    logger.info('... waking up and checking for packets!');
+
+    metrics?.pollCounter?.inc();


With nullish coalescing operator, we can collect metrics nicely w/o condition blocks (if metrics are disabled).

This is neat, but I find it less clear than an if statement.

bartmacbartek · 2021-04-27T22:55:02Z

src/binary/ibc-relayer/setup-prometheus.ts

+    pollCounter: new client.Counter({
+      name: 'poll_counter',
+      help: 'Poll counter',
+    }),


This is only an example of a custom metric that should be removed. Let's replace this with some real ones.

Very good start! Counter makes sense here for this metric. 👍

Some notes for a future version:
I have noticed though that the naming does not fit the others, yet:

confio_relayer_nodejs_gc_duration_seconds_count{kind="minor"} 80 poll_counter 13

With a common prefix we can find all relayer metrics easily. Otherwise some context is missing as a prometheus instance is likely not exclusive to the relayer.

No idea if the relayer is always bidirectional but as this would involve 2 chains the semantics of poll is not fully clear to me. You can use labels to mark which chain you polled.

Naming is always hard. You don't need to carry the type inside the name. Maybe "_count" or "_total" is a better fit. I always found this page very helpful: https://prometheus.io/docs/practices/naming/

Also formats with prettier

willclarktech · 2021-04-28T09:21:07Z

src/binary/ibc-relayer/commands/start.ts

@@ -262,5 +294,7 @@ async function relayerLoop(link: Link, options: Options, logger: Logger) {
    logger.info(`Sleeping ${options.poll} seconds...`);
    await sleep(options.poll * 1000);
    logger.info('... waking up and checking for packets!');
+
+    metrics?.pollCounter?.inc();


This is neat, but I find it less clear than an if statement.

willclarktech · 2021-04-28T10:22:51Z

src/binary/ibc-relayer/setup-prometheus.ts

+  };
+}
+
+export type Metrics = ReturnType<typeof setupPrometheus>;


Why not define the type here and use it in the function definition?

I agree it's safer than relying on the inference.

af3e18c

willclarktech · 2021-04-28T10:23:04Z

src/binary/ibc-relayer/setup-prometheus.ts

+  port,
+  logger,
+}: {
+  enabled: boolean;


Can we make these readonly?

Of course! 4d93ebd

willclarktech · 2021-04-28T10:24:52Z

src/binary/ibc-relayer/setup-prometheus.ts

+
+import { Logger } from '../create-logger';
+
+let initialized = false;


Hm, do we need some kind of global state?

It's just nice to have to make sure we don't initialize the HTTP server more than once: https://github.com/confio/ts-relayer/pull/173/files#diff-95ad111838746b4a0030c8e37ff66c5de9df11b1a2eab5aa3f59895c9f70016cR28-R32

I thought of implementing a singleton here but since it takes arguments like enabled I decided to just throw on second call.

changeset-bot · 2021-04-28T12:16:57Z

🦋 Changeset detected

Latest commit: 19664fd

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

This PR includes changesets to release 1 package

Name	Type
@confio/relayer	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

ethanfrey

Looks good.

A few minor points that can be addressed in a follow up PR. (There are more prometheus tasks)

ethanfrey · 2021-04-28T13:00:44Z

README.md

+
+> **NOTE:** Metrics can also be enabled via an environment variable `RELAYER_ENABLE_METRICS=true`, or with an `enableMetrics: true` entry in the `app.yaml` file, as explained in the [config specification](./spec/config.md#configuration).
+
+The `GET /metrics` endpoint will be exposed by default on port `26660`, which you can override with `--metrics-port` flag, `RELAYER_METRICS_PORT` env variable, or `metricsPort` entry in `app.yaml`.


very nice list of options to set the port. 🌷
Just a note: port 26660 is used by Tendermint as their default port for Prometheus and therefore for any blockchain based on cosmos-sdk like wasmd. This may cause some conflicts when both run on the same machine. 🤷
If you want to change this then I would go with 8080 where people expect conflicts anyway :-)

ethanfrey · 2021-04-28T13:01:32Z

README.md

+1. Start the relayer with metrics enabled
+2. Spin up the Prometheus instance:
+   ```sh
+   docker run -it -v $(pwd):/prometheus -p9090:9090 prom/prometheus --config.file=prometheus.yaml


Very cool, a default prometheus config committed in repo. Better than any docs.

ethanfrey · 2021-04-28T13:03:13Z

prometheus.yaml

+global:
+  scrape_interval: 15s
+  evaluation_interval: 15s
+
+scrape_configs:
+  - job_name: confio_relayer
+
+    scrape_interval: 5s
+    static_configs:
+      - targets: ['host.docker.internal:26660']


Good to commit it to this repo, so it is maintained.

You could put it in a samples or ops dir. We can add example systemd and other configs there as needed.
(Maybe we move it together with the registry.conf file?)

ethanfrey · 2021-04-28T13:04:01Z

src/binary/ibc-relayer/commands/start.ts

-  destConnection?: string;
-  scanFromSrc?: string;
-  scanFromDest?: string;
+  readonly interactive: boolean;


Will was here 😄

ethanfrey · 2021-04-28T13:04:31Z

src/binary/ibc-relayer/commands/start.ts

+    Boolean(process.env.RELAYER_ENABLE_METRICS) ||
+    app?.enableMetrics ||
+    false;
+  const metricsPort = resolveOption('metricsPort', {


ethanfrey · 2021-04-28T13:05:24Z

src/binary/ibc-relayer/commands/start.ts

@@ -171,6 +176,20 @@ export async function start(flags: Flags, logger: Logger) {
    flags.scanFromDest,
    process.env.RELAYER_SCAN_FROM_DEST
  );
+  const enableMetrics =


Why don't you use resolveOption here like everywhere else?

Ah, there is no support for boolean, only string and integer?
If that is the case, leave it for this PR and make an issue to add boolean support, pointing to this line

Exactly!

#174

ethanfrey · 2021-04-28T13:07:07Z

src/binary/ibc-relayer/setup-prometheus.ts

+  logger: Logger;
+}): Metrics {
+  if (initialized) {
+    throw new Error(


This makes sense to find bugs in our code fast.

I was going to suggest to only set initialized to true if enabled was true, but that means double-registry bugs only surface in production. It is good like this

ethanfrey · 2021-04-28T13:08:14Z

src/binary/ibc-relayer/setup-prometheus.ts

+}: {
+  enabled: boolean;
+  port: number;
+  logger: Logger;


Logger is not yet used. Do you plan to use it in this PR or a direct follow up?
If not, can we remove it until there is a clear use? We can always add it back then

It is used here: https://github.com/confio/ts-relayer/pull/173/files#diff-95ad111838746b4a0030c8e37ff66c5de9df11b1a2eab5aa3f59895c9f70016cR57

or do you think it's better to do console log? I would rather keep the logger to make sure the endpoint is mentioned in the logs.

alpe

Good work! 💐 Very happy to see all the system metrics already 🚀

alpe · 2021-04-29T07:55:10Z

README.md

+
+> **NOTE:** Metrics can also be enabled via an environment variable `RELAYER_ENABLE_METRICS=true`, or with an `enableMetrics: true` entry in the `app.yaml` file, as explained in the [config specification](./spec/config.md#configuration).
+
+The `GET /metrics` endpoint will be exposed by default on port `26660`, which you can override with `--metrics-port` flag, `RELAYER_METRICS_PORT` env variable, or `metricsPort` entry in `app.yaml`.


very nice list of options to set the port. 🌷
Just a note: port 26660 is used by Tendermint as their default port for Prometheus and therefore for any blockchain based on cosmos-sdk like wasmd. This may cause some conflicts when both run on the same machine. 🤷
If you want to change this then I would go with 8080 where people expect conflicts anyway :-)

alpe · 2021-04-29T08:07:46Z

src/binary/ibc-relayer/setup-prometheus.ts

+    pollCounter: new client.Counter({
+      name: 'poll_counter',
+      help: 'Poll counter',
+    }),


Very good start! Counter makes sense here for this metric. 👍

Some notes for a future version:
I have noticed though that the naming does not fit the others, yet:

confio_relayer_nodejs_gc_duration_seconds_count{kind="minor"} 80 poll_counter 13

With a common prefix we can find all relayer metrics easily. Otherwise some context is missing as a prometheus instance is likely not exclusive to the relayer.

No idea if the relayer is always bidirectional but as this would involve 2 chains the semantics of poll is not fully clear to me. You can use labels to mark which chain you polled.

Naming is always hard. You don't need to carry the type inside the name. Maybe "_count" or "_total" is a better fit. I always found this page very helpful: https://prometheus.io/docs/practices/naming/

bartmacbartek added 3 commits April 27, 2021 14:51

Install prom-client dependency

069c742

Expose /metrics and add custom counter

417706f

Add 'Monitoring' section to README.md

f997a1b

bartmacbartek commented Apr 27, 2021

View reviewed changes

Remove redundant TODO comment

af01aad

bartmacbartek commented Apr 27, 2021

View reviewed changes

Remove stubbed metrics overkill

6eabfcb

bartmacbartek commented Apr 27, 2021

View reviewed changes

bartmacbartek requested review from willclarktech, alpe and ethanfrey April 27, 2021 22:56

Tidy English in Monitoring section of README

2d0d331

Also formats with prettier

willclarktech reviewed Apr 28, 2021

View reviewed changes

Mark flags and options as readonly

4d93ebd

Define 'setupPrometheus' func return type

af3e18c

ethanfrey approved these changes Apr 28, 2021

View reviewed changes

bartmacbartek and others added 3 commits April 28, 2021 22:19

Move prometheus.yaml example config under demo dir

92287b8

Merge branch 'main' into 133/prometheus

db96856

Create gorgeous-houses-listen.md

19664fd

bartmacbartek merged commit fb5934f into main Apr 28, 2021

bartmacbartek deleted the 133/prometheus branch April 28, 2021 21:15

alpe reviewed Apr 29, 2021

View reviewed changes

bartmacbartek mentioned this pull request Apr 30, 2021

133/minor fixes #175

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

133/prometheus #173

133/prometheus #173

bartmacbartek commented Apr 27, 2021

bartmacbartek Apr 27, 2021

ethanfrey Apr 28, 2021

bartmacbartek Apr 28, 2021

ethanfrey Apr 29, 2021 •

edited

bartmacbartek Apr 27, 2021

bartmacbartek Apr 27, 2021

willclarktech Apr 28, 2021

bartmacbartek Apr 27, 2021 •

edited

alpe Apr 29, 2021 •

edited

willclarktech Apr 28, 2021

willclarktech Apr 28, 2021

bartmacbartek Apr 28, 2021

willclarktech Apr 28, 2021

bartmacbartek Apr 28, 2021

willclarktech Apr 28, 2021

bartmacbartek Apr 28, 2021

changeset-bot bot commented Apr 28, 2021 •

edited

ethanfrey left a comment

ethanfrey Apr 28, 2021

alpe Apr 29, 2021

ethanfrey Apr 28, 2021

ethanfrey Apr 28, 2021

ethanfrey Apr 28, 2021

ethanfrey Apr 28, 2021

ethanfrey Apr 28, 2021

bartmacbartek Apr 28, 2021

ethanfrey Apr 28, 2021

ethanfrey Apr 28, 2021

bartmacbartek Apr 28, 2021

alpe left a comment

alpe Apr 29, 2021

alpe Apr 29, 2021 •

edited


		import { Logger } from '../create-logger';

		let initialized = false;


		> NOTE: Metrics can also be enabled via an environment variable `RELAYER_ENABLE_METRICS=true`, or with an `enableMetrics: true` entry in the `app.yaml` file, as explained in the [config specification](./spec/config.md#configuration).

		The `GET /metrics` endpoint will be exposed by default on port `26660`, which you can override with `--metrics-port` flag, `RELAYER_METRICS_PORT` env variable, or `metricsPort` entry in `app.yaml`.

133/prometheus #173

133/prometheus #173

Conversation

bartmacbartek commented Apr 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey Apr 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartmacbartek Apr 27, 2021 • edited

Choose a reason for hiding this comment

alpe Apr 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Apr 28, 2021 • edited

🦋 Changeset detected

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpe Apr 29, 2021 • edited

Choose a reason for hiding this comment

ethanfrey Apr 29, 2021 •

edited

bartmacbartek Apr 27, 2021 •

edited

alpe Apr 29, 2021 •

edited

changeset-bot bot commented Apr 28, 2021 •

edited

alpe Apr 29, 2021 •

edited