add prometheus metrics#2466
Conversation
|
@tifayuki For simplicity can we split this into 3 separate PRs? I think it will be easier for reviewers and you too :) |
|
@manishtomar |
|
@tifayuki Are you still working on this? |
|
@stevvooe |
| return base.setDriverName(base.StorageDriver.Move(ctx, sourcePath, destPath)) | ||
| start := time.Now() | ||
| err := base.setDriverName(base.StorageDriver.Move(ctx, sourcePath, destPath)) | ||
| prometheus.BlobAction.WithValues("move").UpdateSince(start) |
There was a problem hiding this comment.
This isn't really a blob action. It is a storage driver action.
|
|
||
| var ( | ||
| // BlobAction is the metrics of blob related operations | ||
| BlobAction = storageNamespace.NewLabeledTimer("blob_action", "The number of seconds it takes to get/put blob", "action") |
There was a problem hiding this comment.
These are used for storage actions, not blob actions.
|
|
||
| const ( | ||
| // Namespace is the namespace of prometheus metrics | ||
| Namespace = "docker_distribution" |
There was a problem hiding this comment.
I'd prefer if this were registry or something like that.
There was a problem hiding this comment.
Wait, this isn't actually a namespace: define a single namespace here.
There was a problem hiding this comment.
Or, call it NamespacePrefix to be clear.
| Addr string `yaml:"addr,omitempty"` | ||
| // Prometheus configures the Prometheus telemetry endpoint. | ||
| Prometheus struct { | ||
| Enabled bool `yaml:"enabled,omitempty"` |
There was a problem hiding this comment.
Document what this fields do here and in the documentation.
| @@ -0,0 +1,28 @@ | |||
| package prometheus | |||
There was a problem hiding this comment.
No need to have it be called "prometheus". Just have a single package named metrics.
There was a problem hiding this comment.
Also, rather than defining all the metrics here, just define the namespace here and define the metrics where they are used.
| email: [email protected] | ||
| debug: | ||
| addr: localhost:5001 | ||
| prometheus: |
There was a problem hiding this comment.
There needs to be a section for this.
| addr: localhost:5001 | ||
| prometheus: | ||
| enabled: true | ||
| path: /metrics |
|
|
||
| start := time.Now() | ||
| fi, e := base.StorageDriver.Stat(ctx, path) | ||
| prometheus.BlobAction.WithValues("state").UpdateSince(start) |
| storagedriver "github.com/docker/distribution/registry/storage/driver" | ||
| "time" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Make sure to decorate all the operations. Do we have a way to see writer duration? Writer bytes?
|
|
||
| start := time.Now() | ||
| str, e := base.StorageDriver.List(ctx, path) | ||
| prometheus.BlobAction.WithValues("list").UpdateSince(start) |
There was a problem hiding this comment.
Can these somehow include the name of the driver that is in use? That would be helpful for comparing driver implementations in production.
| BlobAction = storageNamespace.NewLabeledTimer("blob_action", "The number of seconds it takes to get/put blob", "action") | ||
|
|
||
| // CacheRequestCount is the number of total cache request received/hits/misses | ||
| CacheRequestCount = storageNamespace.NewLabeledCounter("cache_request", "The number of cache request received", "type") |
There was a problem hiding this comment.
Cache or something like that is fine. CacheRequest makes me think of counting requests that were cached.
|
As part of this PR, please provide an output of the metrics so we can ensure the format looks correct. |
Codecov Report
@@ Coverage Diff @@
## master #2466 +/- ##
==========================================
- Coverage 60.81% 51.69% -9.13%
==========================================
Files 129 126 -3
Lines 11830 11500 -330
==========================================
- Hits 7195 5945 -1250
- Misses 3733 4810 +1077
+ Partials 902 745 -157
Continue to review full report at Codecov.
|
|
|
||
| var ( | ||
| // storageAction is the metrics of blob related operations | ||
| storageAction = prometheus.StorageNamespace.NewLabeledTimer("storage_action", "The number of seconds that the storage action takes", "driver", "action") |
There was a problem hiding this comment.
This stutters: it becomes registry_storage_storage_action. Should just be action here.
|
sample output with push/pull: |
| handler.ServeHTTP(w, r) | ||
| }) | ||
|
|
||
| httpMetrics := prometheus.RootHTTPNamespace.NewDefaultHttpMetrics("root") |
There was a problem hiding this comment.
I don't think this counter is necessary: it will count for all requests. This can just be rolled up from the other counters.
| return base.setDriverName(base.StorageDriver.PutContent(ctx, path, content)) | ||
| start := time.Now() | ||
| err := base.setDriverName(base.StorageDriver.PutContent(ctx, path, content)) | ||
| storageAction.WithValues(base.Name(), "putcontent").UpdateSince(start) |
There was a problem hiding this comment.
Should use camelcase: PutContent.
40d9313 to
14dc17a
Compare
|
LGTM |
fffacfc to
5752fc4
Compare
89503d6 to
beccba4
Compare
at the first iteration, only the following metrics are collected:
- HTTP metrics of each API endpoint
- cache counter for request/hit/miss
- histogram of storage actions, including:
GetContent, PutContent, Stat, List, Move, and Delete
Signed-off-by: tifayuki <[email protected]>
beccba4 to
e3c37a4
Compare
Signed-off-by: tifayuki [email protected]
depends on:
docker/go-metrics#15
docker/go-metrics#16
cc @nandhini915 @manishtomar