Add notification metrics #2522

tifayuki · 2018-02-13T21:40:13Z

It adds notification related prometheus metrics, including:

total count for events/success/failure/error
total count for notification per each status code
gauge of the pending notification queue

Signed-off-by: tifayuki tifayuki@gmail.com

codecov · 2018-02-13T22:03:59Z

Codecov Report

Merging #2522 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #2522      +/-   ##
=========================================
+ Coverage   60.45%   60.5%   +0.04%     
=========================================
  Files         102     102              
  Lines        8001    8011      +10     
=========================================
+ Hits         4837    4847      +10     
  Misses       2517    2517              
  Partials      647     647

Flag	Coverage Δ
#linux	`60.5% <100%> (+0.04%)`	⬆️

Impacted Files	Coverage Δ
notifications/endpoint.go	`94.44% <100%> (ø)`	⬆️
notifications/metrics.go	`97.14% <100%> (+0.47%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d3efad...92a6436. Read the comment docs.

dmp42 · 2018-03-21T16:42:57Z

Same as the other PR.
Can we see the output?

Thanks.

stevvooe · 2018-03-26T18:15:41Z

metrics/prometheus.go

@@ -10,4 +10,7 @@ const (
 var (
 	// StorageNamespace is the prometheus namespace of blob/cache related operations
 	StorageNamespace = metrics.NewNamespace(NamespacePrefix, "storage", nil)
+
+	// NotificationNamespace is the prometheus namespace of notification related metrics
+	NotificationNamespace = metrics.NewNamespace(NamespacePrefix, "notification", nil)


Pluralize this as notifications.

tifayuki · 2018-03-26T22:16:48Z

An sample output is:

# HELP registry_notifications_events_total The number of total events
# TYPE registry_notifications_events_total counter
registry_notifications_events_total{type="Events"} 8
registry_notifications_events_total{type="Successes"} 8
# HELP registry_notifications_pending_total The gauge of pending events in queue
# TYPE registry_notifications_pending_total gauge
registry_notifications_pending_total 0
# HELP registry_notifications_status_total The number of status code
# TYPE registry_notifications_status_total counter
registry_notifications_status_total{code="200 OK"} 8

@stevvooe @dmp42

mrueg · 2018-04-12T19:27:27Z

@stevvooe any chance to take a look at this? would be great to have this change in.

mrueg · 2018-08-01T08:44:39Z

@dmcgowan @stevvooe friendly ping on this one :)

manishtomar · 2018-08-20T17:13:30Z

@tifayuki Could you add endpoint label also to this PR?

GordonTheTurtle · 2018-10-11T13:39:20Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "notification_metrics" git@github.com:tifayuki/distribution.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354473512
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

tifayuki · 2018-10-11T14:06:37Z

@manishtomar I added the endpoint label. Can you take a look at it?

manishtomar · 2018-10-12T22:45:53Z

@tifayuki Thanks! Code LGTM. Could you merge latest master and push it? I wonder if it will fix travis build.

tifayuki · 2018-10-15T12:39:18Z

@manishtomar
I merged the upstream master to this PR. But on Travis, it always shows:

The command "export CGO_ENABLED=$TRAVIS_CGO_ENABLED" exited with 0.
0.23s$ DCO_VERBOSITY=-q script/validate/dco
/home/travis/gopath/bin/git-validation
 99ce26e - FAIL - has whitespace errors. See `git show --check 99ce26efeda8fc8661974a1d3edaab0740c2abc3`.
1 commits to fix
The command "DCO_VERBOSITY=-q script/validate/dco" exited with 1.

Any idea what error it is?

manishtomar · 2018-10-15T21:20:27Z

@dmp42 Would you know why this is failing? The error is coming for not-recent commit. Should one squash into commit to fix it?

mrueg · 2018-12-13T19:41:07Z

@tifayuki can you rebase on master and see if the build still fails?

tifayuki · 2018-12-14T12:16:53Z

@mrueg it worked

mrueg · 2018-12-14T16:42:09Z

@dmcgowan any chance to get this in for 2.7.1?

manishtomar · 2019-01-31T00:12:22Z

@dmcgowan I added this in 2.7.2 milestone. Any chance this can be merged?

dmcgowan · 2019-02-14T22:16:09Z

LGTM

If you are going to open up a backport to 2.7 for this, please provide strong justification and show how it is not impactful for compatibility. Such a change in a patch release is very irregular, but I understand there is no 2.8 currently on the horizon.

dmcgowan · 2019-02-14T22:17:10Z

Can you squash this before merging? Will make cherry-picking cleaner

manishtomar

@tifayuki Sorry I just realized the mistake in the code. It is incrementing by 1 instead of len(events). Can you please address the mistake and put it up for review again?

notifications/metrics.go

manishtomar · 2019-02-14T23:55:59Z

If you are going to open up a backport to 2.7 for this, please provide strong justification and show how it is not impactful for compatibility.

It does not impact compatibility. I originally wanted it in 2.7 release as this change is being used in Hub registry but missed the train. I thought we could get it in 2.7.1|2 release but I that was assuming that 2.7.x release will be based on master branch which is incorrect. Hence, please ignore my request to put this change in patch release. It can go in next release. I just wanted to get this merged. Also I apologize I just realized a mistake when looking at the code and have requested these. We can merge it after @tifayuki makes those changes. Thanks for your time!

manishtomar · 2019-02-15T06:07:30Z

@tifayuki Thanks for the changes. However build is failing due to unsigned commits which was GH fault when accepting the suggestion. Could you please rebase with one signed commit? Also, you will need to put .Inc(float64(len(events))) to fix the lint error.

It adds notification related prometheus metrics, including: - total count for events/success/failure/error - total count for notification per each status code - gauge of the pending notification queue Signed-off-by: tifayuki <tifayuki@gmail.com>

Signed-off-by: Honglin Feng <tifayuki@gmail.com>

manishtomar

LGTM with a comment. Thanks a lot for this.

notifications/metrics.go

Signed-off-by: Honglin Feng <tifayuki@gmail.com>

manishtomar · 2019-02-19T23:45:15Z

@dmcgowan Can this be merged now please?

thaJeztah · 2019-06-20T05:53:44Z

do we want some of the commits to be squashed before merging?

manishtomar · 2019-06-21T23:44:58Z

@tifayuki Can you please squash commits?

manishtomar · 2019-06-21T23:52:56Z

Actually since this only needs to be merged and not cherry-picked it should be ok to squash during merge @dmcgowan . If thats ok can you or @caervs please merge this?

caervs

LGTM

manishtomar · 2019-06-28T17:11:08Z

@caervs Can you please merge this?

stevvooe reviewed Mar 26, 2018

View reviewed changes

tifayuki force-pushed the notification_metrics branch from 0dfd56e to 636c645 Compare March 26, 2018 21:53

GordonTheTurtle added the dco/no label Oct 11, 2018

tifayuki force-pushed the notification_metrics branch from 0eb92cb to e2f0f57 Compare October 11, 2018 13:41

GordonTheTurtle removed the dco/no label Oct 11, 2018

GordonTheTurtle added the dco/no label Oct 15, 2018

tifayuki force-pushed the notification_metrics branch from 78ec108 to 99ce26e Compare October 15, 2018 10:25

GordonTheTurtle removed the dco/no label Oct 15, 2018

tifayuki force-pushed the notification_metrics branch 2 times, most recently from 9f514b8 to a3d6f72 Compare October 15, 2018 12:28

tifayuki force-pushed the notification_metrics branch from a3d6f72 to 5c66b57 Compare December 14, 2018 12:03

manishtomar added this to the Registry/2.7.2 milestone Jan 31, 2019

manishtomar suggested changes Feb 14, 2019

View reviewed changes

notifications/metrics.go Outdated Show resolved Hide resolved

notifications/metrics.go Outdated Show resolved Hide resolved

notifications/metrics.go Outdated Show resolved Hide resolved

notifications/metrics.go Outdated Show resolved Hide resolved

GordonTheTurtle added the dco/no label Feb 15, 2019

tifayuki added 4 commits February 15, 2019 21:14

Add notification metrics

8b70616

It adds notification related prometheus metrics, including: - total count for events/success/failure/error - total count for notification per each status code - gauge of the pending notification queue Signed-off-by: tifayuki <tifayuki@gmail.com>

add label to the metrics

76da629

Signed-off-by: Honglin Feng <tifayuki@gmail.com>

run go fmt

228bafc

Signed-off-by: Honglin Feng <tifayuki@gmail.com>

run go fmt and goimports

09a63ca

Signed-off-by: Honglin Feng <tifayuki@gmail.com>

tifayuki force-pushed the notification_metrics branch from e1663b8 to b0c2c80 Compare February 15, 2019 13:15

update the event number

d5a615b

Signed-off-by: Honglin Feng <tifayuki@gmail.com>

tifayuki force-pushed the notification_metrics branch from b0c2c80 to d5a615b Compare February 15, 2019 13:55

GordonTheTurtle removed the dco/no label Feb 15, 2019

manishtomar approved these changes Feb 17, 2019

View reviewed changes

notifications/metrics.go Outdated Show resolved Hide resolved

rename the metrics label

92a6436

Signed-off-by: Honglin Feng <tifayuki@gmail.com>

caervs approved these changes Jun 28, 2019

View reviewed changes

caervs merged commit be07be9 into distribution:master Jun 28, 2019

Add notification metrics #2522

Add notification metrics #2522

Uh oh!

Conversation

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!