feat: add metric counting using OTL #15
Conversation
Dockerfile.launchContract
Outdated
| RUN forge install openzeppelin/openzeppelin-contracts --no-git | ||
| RUN forge build --use 0.8.17 | ||
| RUN forge install openzeppelin/openzeppelin-contracts@v4.8.0 --no-git | ||
| RUN forge build --use 0.8.13 | ||
| ENTRYPOINT forge create --contracts ./src/CeramicAnchorServiceV2.sol --private-key $ETH_WALLET_PK --rpc-url $ETH_RPC_URL CeramicAnchorServiceV2 |
There was a problem hiding this comment.
Mind adding a newline at the end of this file?
models/constants.go
Outdated
|
|
||
| // Metrics | ||
| // Counts | ||
| const CreatedBatchMetricName = "created_batch" |
There was a problem hiding this comment.
Should we make these into an enum?
type MetricName string
const (
MetricName_CreatedBatch MetricName = "created_batch"
MetricName_ReplacedRequest MetricName = "replaced_request"
)
There was a problem hiding this comment.
Do we need additional metrics?
- Ingress requests for the Validation service so we can see how much load we're receiving
- Ingress requests to the Batching service so we can see how many requests got past deduplication
- Avg. number of requests in a Batch so we can see the pattern of batch sizes over time under different loads
- Avg. batch linger (need to check if this is possible to measure) so we can see the time it's taking for batches to be issued.
- Number of anchor jobs created by the Worker service
- Number of failures received by the Failure Handler
- Others?
Don't have to implement all these in this PR though we should track this list somewhere and add them in subsequent PRs.
There was a problem hiding this comment.
Also question for @gvelez17 here - when we replace the CASv2 Scheduler with the CASv5 Scheduler, will we lose any metrics you might have added to the old Scheduler? If so, then we probably need to make sure that at least those metrics, if still useful in the new design, are retained in the new code.
There was a problem hiding this comment.
I'll make a ticket
common/metrics/otl.go
Outdated
| "go.opentelemetry.io/otel/sdk/resource" | ||
| ) | ||
|
|
||
| type OTLMetricService struct { |
There was a problem hiding this comment.
minor: Should this be named OtlMetricService?
There was a problem hiding this comment.
i think not in case we change the internals later
There was a problem hiding this comment.
I'm not sure I understand. Was just recommending changing OTL -> Otl to preserve camel casing. Should we not?
common/metrics/otl.go
Outdated
| } | ||
|
|
||
| var exporter sdk.Exporter | ||
| collectorUrl := os.Getenv("COLLECTOR_URL") |
There was a problem hiding this comment.
the actual env variable is COLLECTOR_HOST
There was a problem hiding this comment.
we normally pass this to the observability package from the caller
common/metrics/otl.go
Outdated
| sdk.WithReader(sdk.NewPeriodicReader(exporter)), | ||
| ) | ||
|
|
||
| meter := meterProvider.Meter("github.com/ceramicnetwork/go-cas/common/metrics") |
There was a problem hiding this comment.
maybe just 'go-cas'
later we can move this to a go observability repo and use caller again
common/metrics/otl.go
Outdated
| "encoding/json" | ||
| "os" | ||
|
|
||
| "github.com/ceramicnetwork/go-cas/models" |
There was a problem hiding this comment.
minor: Move this line to a new section after the imports below separated by a newline
No description provided.