PLEX-2935/generalize monitoring#2132
Conversation
|
Unheilbar
changed the title
| return err | ||
| } | ||
|
|
||
| withMonitoring := os.Getenv("CHAINLINK_PROTOC_WITH_MONITORING") == "true" |
There was a problem hiding this comment.
Is there precedent for the CHAINLINK_ prefix? Because we use just CL_ in many places.
| mc := c.ClientCapability.MonitoringContext() | ||
| kvs := []any{"method", "GetAccountInfoWithOpts", "executionID", metadata.WorkflowExecutionID} | ||
| if labeled, ok := any(input).(capabilities.MonitoringLabels); ok { | ||
| kvs = append(kvs, labeled.KVs()...) |
There was a problem hiding this comment.
We should separate KVs for logging and KVs for metrics.
In the case of metrics, we cannot use high-cardinality values, as it will overload metrics storage.
| // on the generated ClientCapability interface. | ||
| type MonitoringContext struct { | ||
| Logger logger.Logger | ||
| Processor beholder.ProtoProcessor |
There was a problem hiding this comment.
We no longer need to use proto processor. Previously, beholder events were the only way to observe logs from a NOP. Now, with logs streaming, it's redundant for chain capabilities.
There was a problem hiding this comment.
Also we should be able to define metrics (need both beholder and prom) on common level
| // capability lifecycle event. It intentionally mirrors | ||
| // capabilities/libs/monitoring.ExecutionContext in the capabilities binary repo | ||
| // so that beholder consumers see a consistent schema. | ||
| message ExecutionContext { |
There was a problem hiding this comment.
proto is redundant as we do not need to use beholder
There was a problem hiding this comment.
We also should not ask for the capability to populate this whole structure.
It's sufficient to expose a general MetricsAttributes method that includes capability-specific labels like chain_id, name, etc.
The rest of the fields can be provided by the generated code
| @@ -0,0 +1,295 @@ | |||
| package server | |||
There was a problem hiding this comment.
Why do we need a separate template to enable monitoring? It contains a lot of duplicate code, and if we introduce another optional feature, it will force us to double templates.
Can't we optionally generate monitoring code by passing a variable to the template?
3 participants
JIRA
MonitoringContext() implementation by solana capability here
Summary
--with-monitoringto capability server generation for per-capability opt-inCapabilityActionInitiated,CapabilityActionSuccess,CapabilityActionErrorKVs() []anyon request types for method-specific log enrichment (e.g. pkg/capabilities/v2/chain-capabilities/solana/monitoring_kvs.go)MonitoringContext()on generated capability interfacesBenefits
go generate --with-monitoringmethodlabel instead of maintaining action listschainlink-commonstays lightweight: onlyMonitoringLabelsin base packageNotes
WriteReportTxFeeCalculationError) and trigger monitoring remain manualexecution_context.protoregistration panic