Add OTel metrics #98

siburu · 2023-08-21T13:14:36Z

No description provided.

mizuochik · 2023-08-22T09:49:38Z

core/metric.go

+	name := fmt.Sprintf("%s.processed_block_height", namespaceRoot)
+	callback := func(ctx context.Context, observer api.Int64Observer) error {
+		observer.Observe(metricSrcProcessedBlockHeight.Load(), api.WithAttributes(attribute.String("chain", "src")))
+		observer.Observe(metricDstProcessedBlockHeight.Load(), api.WithAttributes(attribute.String("chain", "dst")))


Adding attributes expressing more detailed context?

Being ambiguous with chain:src and chain:dst only. A relayer process treats multiple src/dest pairs.

We may add attributes like chain_id:xxx and direction:src|dest.

Done:

https://github.com/hyperledger-labs/yui-relayer/pull/98/files#diff-603919a5f90c94114502631746a1fae0f788492505b9cf82272b47afab8fcfaaR94-R103

https://github.com/hyperledger-labs/yui-relayer/pull/98/files#diff-93cd18f6e37c22009ecb3b8eba7e6ab49e068873cc99b091fc311cc9dfb6bf98R431-R438

mizuochik · 2023-08-22T10:01:18Z

core/metric.go

+
+	// create the instrument "relayer.processed_block_height"
+	name := fmt.Sprintf("%s.processed_block_height", namespaceRoot)
+	callback := func(ctx context.Context, observer api.Int64Observer) error {


Optional: How about writing callbacks more directly as follows?

Define func Observe(ctx context.Context, observer api.Observer) on SyncHeaders

Make InitializeMetrics to take a SyncHeaders value and register SyncHeaders.Observe as callback

The advantage is avoiding global atomic variables metricXXX and reducing codes treat them.

Thank you for your good suggestion! However there is a reason why I can't adopt this soon.

An instance of SyncHeaders is created, used, and dropped for each relay cycle.
Therefore it is meaningless to observer SyncHeaders to obtain metrics.
Not only SyncHeaders but all other states in the relayer are also short-lived and not held longer than one relay cycle.
Therefore we have to add some "global variables" to store metrics.

I forgot to say that we basically want to keep the core structure of the relayer stateless.

I noticed my big mistake ... when I tried to move metric.go to another directory.
metric.go can be referred to by any other components in the relayer.
Therefore, to avoid cyclic reference, metric.go itself should not depends on any types or functions in the relayer.

The correct structure seems the following:

The metrics subsystem (metric.go) keeps:

The instance of MeterProvider

The instance of Meter

The instruments like Counters and Gauges

Since we want to easily list up all metrics exported by the relayer, I put them together.

The other components (e.g. relayer core, tendermint module, mock prover) keeps:

Original data needed to calculate final metrics values

Logics to calculate final metrics values from original data

Thanks for explaining! I have a question and a comment. (These aren't fixing requests. I agree the direction implementing core logics stateless.)

Question

Are you saying that SyncHeader is not ALWAYS long-lived?

SyncHeader seems long-lived structure when a relayer starts in service mode.

https://github.com/siburu/yui-relayer/blob/e6c5f9eca4fbde03c04218149776f068e948bb1d/core/service.go#L13
https://github.com/siburu/yui-relayer/blob/e6c5f9eca4fbde03c04218149776f068e948bb1d/core/naive-strategy.go#L65

It's OK that metrics are availbable only running in service mode.

Comment

We can also avoid cyclic reference by defining an interface like type SubjectI interface { Observe() } at metrics.go. (like here)
Other types depend on SubjectI. metrics.go will keep independence from other go files/packages.

@mizuochik
Right. I was wrong about the lifespan of SyncHeader.

Please forget the 3 comments that start from #98 (comment).
My current position is #98 (comment).

I think all the gauges that I added are more suitable for synchronous gauge, but the current release of OTel-go doesn't support synchronous gauge yet.
Therefore I defined our own implementation of synchronous gauge in metrics/sync_gauge.go.
As a result, I didn't need to implement Observe in SyncHeader or NaiveStrategy.

I believe synchronous gauge will be supported by OTel-go in the near future.
open-telemetry/opentelemetry-specification#3540
At that time, we can easily replace metrics/sync_gauge.go with the official implementation.

Thanks. I agreed!

SyncGauge seems suitable to me as well.

mizuochik · 2023-08-22T15:59:24Z

core/metric.go

+
+	// create the instrument "relayer.backlog_size"
+	name = fmt.Sprintf("%s.backlog_size", namespaceRoot)
+	callback = func(ctx context.Context, observer api.Int64Observer) error {


Optional: Same as above.

We may move this callback and the backlog variables to NaiveStrategy's method and fields.

mizuochik · 2023-08-22T16:02:09Z

core/metric.go

+	name = fmt.Sprintf("%s.backlog_size", namespaceRoot)
+	callback = func(ctx context.Context, observer api.Int64Observer) error {
+		observer.Observe(metricSrcBacklogSize.Load(), api.WithAttributes(attribute.String("chain", "src")))
+		observer.Observe(metricDstBacklogSize.Load(), api.WithAttributes(attribute.String("chain", "dst")))


Same as processed_block_height.

mizuochik · 2023-08-22T16:08:36Z

core/metric.go

+
+	// create the instrument "relayer.backlog_oldest_height"
+	name = fmt.Sprintf("%s.backlog_oldest_height", namespaceRoot)
+	callback = func(ctx context.Context, observer api.Int64Observer) error {


Optional: Same as above.

This callback also able to be move to NaiveStrategy.

In summary, observable callbacks may be implemented as follows.

metrics.go

// // Measurement subject // type SubjectI struct { Observe(ctx context.Context, observer metricapi.Observer) error } func InitializeMetrics(....., subjects SubjectI...) error { // Taking measurement subjects (syncHeaders, naiveStrategy, etc...) // Initialize metric providers, instruments, etc... : // Register measurement subjects (syncHeaders, naiveStrategy, etc...) for _, subject := range subjects { if _, err := meter.RegisterCallback( subject.Observe, ProcessedBlockHeightGauge, BacklogSizeGauge, BacklogOldestHeightGauge, ..... // rest observable instruments ); err != nil { panic(err) } } : }

headers.go

func (sh *syncHeaders) Observe(ctx context.Context, observer api.Observer) error { : observer.Observe(ProcessedBlockHeightGauge, .....) : }

naive-strategy.go

type NaiveStrategy struct { : // backlogs may have to be keep here srcBacklog PacketInfoList destBacklog PacketInfoList } : // The below callback taking generic `api.Observer` (not like `api.Int64Observer`) // handles *multiple* observable gauges at once. func (st *NaiveStrategy) Observe(ctx context.Context, observer api.Observer) error { : observer.Observe(BacklogSizeGauge, ...) : observer.Observe(BacklogOldestHeightGauge, ...) : } func (st NaiveStrategy) UnrelayedPackets(src, dst *ProvableChain, sh SyncHeaders, includeRelayedButUnfinalized bool) (*RelayPackets, error) { : ReceivePacketsFinalizedCounter.Add(1) // This counter is non-observable, so increment in main relayer logics directly : }

mizuochik · 2023-08-23T04:52:10Z

core/metric.go

+	instProcessedBlockHeight    api.Int64ObservableGauge
+	instBacklogSize             api.Int64ObservableGauge
+	instBacklogOldestHeight     api.Int64ObservableGauge
+	instReceivePacketsFinalized api.Int64Counter


Optional: Naming xxxGauge and xxxCounter may be better.

Gauge and Counter are domain-specific terms around observability, so better express the intent of variables.

Fixed: https://github.com/hyperledger-labs/yui-relayer/pull/98/files#diff-d715f391f2ffa038c3167bfea1adf741c1f5e39f08349fbbacab780a594146a4R25-R28

mizuochik · 2023-08-23T09:29:53Z

core/metric.go

+			instReceivePacketsFinalized.Add(ctx, 1, api.WithAttributes(attribute.String("chain", "dst")))
+		}
+	}
+	metricDstBacklog = newDstBacklog


Are there any QA or tests for the above logic?

mizuochik · 2023-08-23T10:22:17Z

core/metric.go

+	name = fmt.Sprintf("%s.backlog_oldest_height", namespaceRoot)
+	callback = func(ctx context.Context, observer api.Int64Observer) error {
+		observer.Observe(metricSrcBacklogOldestHeight.Load(), api.WithAttributes(attribute.String("chain", "src")))
+		observer.Observe(metricDstBacklogOldestHeight.Load(), api.WithAttributes(attribute.String("chain", "dst")))


Same as processed_block_height.

mizuochik · 2023-08-23T10:23:36Z

core/metric.go

+	go func() {
+		mux := http.NewServeMux()
+		mux.Handle("/metrics", promhttp.Handler())
+		if err := http.ListenAndServe(addr, mux); err != nil && err != http.ErrServerClosed {


If the above handler stop, the entire process never stops. Metrics will be lost even if the process is alive.

We want relayer processes stopping when metrics become unavailable.
Cloud infrastructures like K8s heal those crashed processes by restarting.

Fixed the error handling here to panic on any types of errors.
https://github.com/hyperledger-labs/yui-relayer/pull/98/files#diff-d715f391f2ffa038c3167bfea1adf741c1f5e39f08349fbbacab780a594146a4R101

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

… down Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

mizuochik · 2023-08-29T00:31:42Z

@siburu Thanks for fixing. I commented.
#98 (comment)

LGTM about the others!

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

…rometheus-addr` field from the global config Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

bluele · 2023-08-30T06:21:18Z

metrics/metrics.go

+	meterProvider *metric.MeterProvider
+	meter         api.Meter
+
+	ProcessedBlockHeightGauge      *Int64SyncGauge


@siburu
It would be helpful to write a succinct description for each metric here.

oh, I noticed that they are written in the initialization of them.

Yes but I should move them to here! Thank you.

bluele

LGTM👍

siburu marked this pull request as ready for review August 21, 2023 15:11

siburu requested a review from a team as a code owner August 21, 2023 15:11

siburu requested a review from bluele August 21, 2023 15:11

mizuochik suggested changes Aug 23, 2023

View reviewed changes

siburu added 8 commits August 25, 2023 10:58

add the metrics subsystem

f25d78c

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

fix the metrics subsystem

a3c328e

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

add relayer.{src,dst}.processed_block_height metrics

ed99edc

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

add relayer.{src,dst}.backlog_size metrics

97e0d49

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

add relayer.{src,dst}.backlog_oldest_height metrics

60607ca

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

add relayer.receive_packets_finalized metrics

10fdc01

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

refactor metrics

6518cdf

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

update go.mod

f1d5fe5

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

siburu force-pushed the add-otel-metrics branch from 9029439 to f1d5fe5 Compare August 25, 2023 02:07

siburu added 5 commits August 25, 2023 11:18

replace metric "backlog_oldest_height" with "backlog_oldest_timestamp"

05f5bdb

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

refactor metrics

86263b3

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

mv the metrics subsystem to the dedicated directory

4a71651

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

fix to shut down the relayer when the metrics exporter server is shut…

f05a220

… down Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

add PacketInfoList::Subtract function with unit testing for it

e6c5f9e

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

siburu requested a review from mizuochik August 28, 2023 03:12

mizuochik approved these changes Aug 29, 2023

View reviewed changes

siburu added 3 commits August 30, 2023 00:15

fix to run the prom exporter only on yrly service start

3c2f172

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

add "--prometheus-addr" flag to service start command and remove `p…

836a8dc

…rometheus-addr` field from the global config Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

add type guards for ExporterConfig implementations

5ee29bd

Signed-off-by: Masanori Yoshida <masanori.yoshida@datachain.jp>

bluele reviewed Aug 30, 2023

View reviewed changes

bluele approved these changes Aug 30, 2023

View reviewed changes

siburu merged commit 10824d7 into hyperledger-labs:main Aug 30, 2023
5 checks passed

siburu deleted the add-otel-metrics branch October 16, 2023 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OTel metrics #98

Add OTel metrics #98

siburu commented Aug 21, 2023

mizuochik Aug 22, 2023 •

edited

Loading

siburu Aug 28, 2023

mizuochik Aug 22, 2023 •

edited

Loading

siburu Aug 25, 2023 •

edited

Loading

siburu Aug 25, 2023

siburu Aug 25, 2023

siburu Aug 25, 2023 •

edited

Loading

mizuochik Aug 29, 2023 •

edited

Loading

siburu Aug 29, 2023 •

edited

Loading

mizuochik Aug 29, 2023

mizuochik Aug 22, 2023

mizuochik Aug 22, 2023

mizuochik Aug 22, 2023 •

edited

Loading

mizuochik Aug 23, 2023 •

edited

Loading

siburu Aug 28, 2023

mizuochik Aug 23, 2023

mizuochik Aug 23, 2023

mizuochik Aug 23, 2023 •

edited

Loading

siburu Aug 28, 2023

mizuochik commented Aug 29, 2023

bluele Aug 30, 2023

bluele Aug 30, 2023

siburu Aug 30, 2023 •

edited

Loading

bluele left a comment

Add OTel metrics #98

Add OTel metrics #98

Conversation

siburu commented Aug 21, 2023

mizuochik Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mizuochik Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

siburu Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siburu Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

mizuochik Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Question

Comment

siburu Aug 29, 2023 • edited Loading

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

mizuochik Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

metrics.go

headers.go

naive-strategy.go

mizuochik Aug 23, 2023 • edited Loading

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

mizuochik Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mizuochik commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siburu Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

bluele left a comment

Choose a reason for hiding this comment

mizuochik Aug 22, 2023 •

edited

Loading

mizuochik Aug 22, 2023 •

edited

Loading

siburu Aug 25, 2023 •

edited

Loading

siburu Aug 25, 2023 •

edited

Loading

mizuochik Aug 29, 2023 •

edited

Loading

siburu Aug 29, 2023 •

edited

Loading

mizuochik Aug 22, 2023 •

edited

Loading

mizuochik Aug 23, 2023 •

edited

Loading

mizuochik Aug 23, 2023 •

edited

Loading

siburu Aug 30, 2023 •

edited

Loading