feat: wrap github api updates with metric counter (#5)

the plugins/settings tend to all handle updates different, sometimes
catching errors using the promises, sometimes propagating, sometimes
catching with a try/catch block. This lack of consistency makes it very
hard to create a clean metric abstraction layer.

As an alternative, have created a sort of higher order function for
wrapping plugin github api calls with a metric. We can decorate all the
github update calls and it will increment a counter based on the call
result and propagate any errors up the stack.

Not ideal, but cognitively fairly simple, and the changes are just
around octokits, which hopefully are unlikely to change (the params
might but we dont interfere with that). We pass in the plugins so we can
refer to their name and access the repoName that is part of the
instance.

Not sure how best to test this, as loads of the tests fail and tried
fixing them quickly but no luck. I have tested locally with our
safe-settings local dev setup in k8s and seen the metrics perform
better. Some functionality is only available on orgs so will need some
additional testing in sbx

lib/metrics.js

@@ -1,7 +1,33 @@
 const metrics = require("@operate-first/probot-metrics");
 
-module.exports =  metrics.useCounter({
-    name: 'num_of_actions_total',
-    help: 'Total number of actions received',
-    labelNames: ['repository', 'result'],
+const counter = metrics.useCounter({
+  name: 'num_of_actions_total',
+  help: 'Total number of actions received',
+  labelNames: ['repository', 'result', 'action'],
 });
+
+const meteredPlugin = async (plugin, fn) => {
+  try {
+    const result = await fn()
+    counter
+      .labels({
+        repository: plugin.repo.repo,
+        result: "success",
+        action: plugin.constructor.name
+      })
+      .inc();
+    return result
+  } catch (e) {
+    console.dir(e)
+    counter
+      .labels({
+        repository: plugin.repo.repo,
+        result: "error",
+        action: plugin.constructor.name
+      })
+      .inc();
+    throw e;
+  }
+}
+
+module.exports = {meteredPlugin}

lib/plugins/autolinks.js

@@ -1,6 +1,7 @@
 /* eslint-disable camelcase */
 const Diffable = require('./diffable')
 const NopCommand = require('../nopcommand')
+const { meteredPlugin } = require('../metrics')
 
 module.exports = class Autolinks extends Diffable {
   // constructor (...args) {
@@ -49,7 +50,7 @@ module.exports = class Autolinks extends Diffable {
     }
 
     try {
-      return this.github.repos.createAutolink(attrs)
+      return meteredPlugin(this, () => this.github.repos.createAutolink(attrs))
     } catch (e) {
       if (e?.response?.data?.errors?.[0]?.code === 'already_exists') {
         this.log.debug(`Did not update ${key_prefix}, as it already exists`)
@@ -72,6 +73,6 @@ module.exports = class Autolinks extends Diffable {
         'Remove autolink'
       )
     }
-    return this.github.repos.deleteAutolink(attrs)
+    return meteredPlugin(this, () => this.github.repos.deleteAutolink(attrs))
   }
 }

lib/plugins/branches.js

@@ -1,6 +1,6 @@
 const NopCommand = require('../nopcommand')
 const MergeDeep = require('../mergeDeep')
-const metric = require("../metrics");
+const { meteredPlugin } = require('../metrics')
 const ignorableFields = []
 const previewHeaders = { accept: 'application/vnd.github.hellcat-preview+json,application/vnd.github.luke-cage-preview+json,application/vnd.github.zzzax-preview+json' }
 
@@ -36,7 +36,7 @@ module.exports = class Branches {
                 return Promise.resolve(resArray)
               }
 
-              return this.github.repos.deleteBranchProtection(params).catch(e => { return [] })
+              return meteredPlugin(this, () => this.github.repos.deleteBranchProtection(params)).catch(e => { return [] })
             } else {
               // Branch protection is not empty
               let p = Object.assign(this.repo, { branch: branch.name })
@@ -73,7 +73,7 @@ module.exports = class Branches {
                   return Promise.resolve(resArray)
                 }
                 this.log.debug(`Adding branch protection ${JSON.stringify(params)}`)
-                return this.github.repos.updateBranchProtection(params).then(res => this.log(`Branch protection applied successfully ${JSON.stringify(res.url)}`)).catch(e => { this.log.error(`Error applying branch protection ${JSON.stringify(e)}`); return [] })
+                return meteredPlugin(this, () =>this.github.repos.updateBranchProtection(params)).then(res => this.log(`Branch protection applied successfully ${JSON.stringify(res.url)}`)).catch(e => { this.log.error(`Error applying branch protection ${JSON.stringify(e)}`); return [] })
               }).catch((e) => {
                 if (e.status === 404) {
                   Object.assign(params, branch.protection, { headers: previewHeaders })
@@ -82,14 +82,8 @@ module.exports = class Branches {
                     return Promise.resolve(resArray)
                   }
                   this.log.debug(`Adding branch protection ${JSON.stringify(params)}`)
-                  return this.github.repos.updateBranchProtection(params).then(res => this.log(`Branch protection applied successfully ${JSON.stringify(res.url)}`)).catch(e => { this.log.error(`Error applying branch protection ${JSON.stringify(e)}`); return [] })
+                  return meteredPlugin(this, () => this.github.repos.updateBranchProtection(params)).then(res => this.log(`Branch protection applied successfully ${JSON.stringify(res.url)}`)).catch(e => { this.log.error(`Error applying branch protection ${JSON.stringify(e)}`); return [] })
                 } else {
-                  metric
-                      .labels({
-                        repository: this.repo.repo,
-                        result: 'error',
-                      })
-                      .inc();
                   this.log.error(e)
                   // return
                 }

lib/plugins/collaborators.js

@@ -1,6 +1,7 @@
 /* eslint-disable camelcase */
 const Diffable = require('./diffable')
 const NopCommand = require('../nopcommand')
+const { meteredPlugin } = require('../metrics')
 
 module.exports = class Collaborators extends Diffable {
   constructor (...args) {
@@ -75,7 +76,7 @@ module.exports = class Collaborators extends Diffable {
         new NopCommand(this.constructor.name, this.repo, this.github.repos.addCollaborator.endpoint(data), 'Add Collaborators')
       ])
     }
-    return this.github.repos.addCollaborator(data)
+    return meteredPlugin(this, () => this.github.repos.addCollaborator(data))
   }
 
   updateInvite (invitation_id, permissions) {
@@ -90,7 +91,7 @@ module.exports = class Collaborators extends Diffable {
         new NopCommand(this.constructor.name, this.repo, this.github.repos.updateInvitation.endpoint(data), 'Update Invitation')
       ])
     }
-    return this.github.repos.updateInvitation(data)
+    return meteredPlugin(this, () => this.github.repos.updateInvitation(data))
   }
 
   remove (existing) {
@@ -102,7 +103,7 @@ module.exports = class Collaborators extends Diffable {
             'Delete Invitation')
         ])
       }
-      return this.github.repos.deleteInvitation(data)
+      return meteredPlugin(this, () => this.github.repos.deleteInvitation(data))
     } else {
       const data = Object.assign({ username: existing.username }, this.repo)
       if (this.nop) {
@@ -111,7 +112,7 @@ module.exports = class Collaborators extends Diffable {
             'Remove Collaborator')
         ])
       }
-      return this.github.repos.removeCollaborator(data)
+      return meteredPlugin(this, () => this.github.repos.removeCollaborator(data))
     }
   }
 }

lib/plugins/labels.js

@@ -1,5 +1,6 @@
 const Diffable = require('./diffable')
 const NopCommand = require('../nopcommand')
+const { meteredPlugin } = require('../metrics')
 const previewHeaders = { accept: 'application/vnd.github.symmetra-preview+json' }
 
 module.exports = class Labels extends Diffable {
@@ -66,7 +67,7 @@ module.exports = class Labels extends Diffable {
         new NopCommand(this.constructor.name, this.repo, this.github.issues.updateLabel.endpoint(this.wrapAttrs(attrs)), 'Update label')
       ])
     }
-    return this.github.issues.updateLabel(this.wrapAttrs(attrs))
+    return meteredPlugin(this, () => this.github.issues.updateLabel(this.wrapAttrs(attrs)))
   }
 
   add (attrs) {
@@ -76,7 +77,7 @@ module.exports = class Labels extends Diffable {
       ])
     }
     this.log.debug(`Creating labels for ${JSON.stringify(attrs, null, 4)}`)
-    return this.github.issues.createLabel(this.wrapAttrs(attrs)).catch(e => this.log.error(` ${JSON.stringify(e)}`))
+    return meteredPlugin(this, () => this.github.issues.createLabel(this.wrapAttrs(attrs)).catch(e => this.log.error(` ${JSON.stringify(e)}`)))
   }
 
   remove (existing) {
@@ -88,7 +89,7 @@ module.exports = class Labels extends Diffable {
         new NopCommand(this.constructor.name, this.repo, this.github.issues.deleteLabel.endpoint(this.wrapAttrs({ name: existing.name })), 'Delete label')
       ])
     }
-    return this.github.issues.deleteLabel(this.wrapAttrs({ name: existing.name }))
+    return meteredPlugin(this, () => this.github.issues.deleteLabel(this.wrapAttrs({ name: existing.name })))
   }
 
   wrapAttrs (attrs) {

lib/plugins/milestones.js

@@ -1,4 +1,5 @@
 const Diffable = require('./diffable')
+const { meteredPlugin } = require('../metrics')
 
 module.exports = class Milestones extends Diffable {
   constructor (...args) {
@@ -29,18 +30,18 @@ module.exports = class Milestones extends Diffable {
   update (existing, attrs) {
     const { owner, repo } = this.repo
 
-    return this.github.issues.updateMilestone(Object.assign({ milestone_number: existing.number }, attrs, { owner, repo }))
+    return meteredPlugin(this, () => this.github.issues.updateMilestone(Object.assign({ milestone_number: existing.number }, attrs, { owner, repo })))
   }
 
   add (attrs) {
     const { owner, repo } = this.repo
 
-    return this.github.issues.createMilestone(Object.assign({}, attrs, { owner, repo }))
+    return meteredPlugin(this, () => this.github.issues.createMilestone(Object.assign({}, attrs, { owner, repo })))
   }
 
   remove (existing) {
     const { owner, repo } = this.repo
 
-    return this.github.issues.deleteMilestone(Object.assign({ milestone_number: existing.number }, { owner, repo }))
+    return meteredPlugin(this, () => this.github.issues.deleteMilestone(Object.assign({ milestone_number: existing.number }, { owner, repo })))
   }
 }

lib/plugins/repository.js

@@ -2,7 +2,7 @@
 // const EndPoints = require('@octokit/plugin-rest-endpoint-methods')
 const NopCommand = require('../nopcommand')
 const MergeDeep = require('../mergeDeep')
-const metric = require("../metrics");
+const { meteredPlugin } = require('../metrics')
 const ignorableFields = [
   'id',
   'node_id',
@@ -57,7 +57,6 @@ module.exports = class Repository {
   }
 
   sync () {
-
     const resArray = []
     this.log.debug(`Syncing Repo ${this.settings.name}`)
     this.settings.name = this.settings.name || this.settings.repo
@@ -152,12 +151,6 @@ module.exports = class Repository {
             }
           }
         } else {
-          metric
-              .labels({
-                repository: this.repo.repo,
-                result: 'error',
-              })
-              .inc();
           this.log.error(` Error ${JSON.stringify(e)}`)
         }
       })
@@ -180,7 +173,7 @@ module.exports = class Repository {
         resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.repos.update.endpoint(parms), 'Update Repo'))
       } else {
         this.log.debug(`Updating repo with settings ${JSON.stringify(parms)}`)
-        return this.github.repos.update(parms)
+        return meteredPlugin(this, () => this.github.repos.update(parms))
       }
     }).catch(e => {
       if (e.status === 404) {
@@ -195,15 +188,9 @@ module.exports = class Repository {
         if (this.nop) {
           resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.repos.renameBranch.endpoint(oldname, this.settings.default_branch), `Repo rename default branch to ${this.settings.default_branch}`))
         } else {
-          return this.github.repos.renameBranch(parms)
+          return meteredPlugin(this, () => this.github.repos.renameBranch(parms))
         }
       } else {
-        metric
-            .labels({
-              repository: this.settings.repo,
-              result: 'error',
-            })
-            .inc();
         this.log.error(`Error ${JSON.stringify(e)}`)
       }
     })
@@ -215,7 +202,7 @@ module.exports = class Repository {
       resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.repos.update.endpoint(this.settings), 'Update Repo'))
       return Promise.resolve(resArray)
     }
-    return this.github.repos.update(this.settings)
+    return meteredPlugin(this, () => this.github.repos.update(this.settings))
   }
 
   updatetopics (repoData, resArray) {
@@ -236,7 +223,7 @@ module.exports = class Repository {
           resArray.push((new NopCommand(this.constructor.name, this.repo, this.github.repos.replaceAllTopics.endpoint(parms), 'Update Topics')))
           return Promise.resolve(resArray)
         }
-        return this.github.repos.replaceAllTopics(parms)
+        return meteredPlugin(this, () => this.github.repos.replaceAllTopics(parms))
       } else {
         this.log(`no need to update topics for ${repoData.data.name}`)
         if (this.nop) {
@@ -260,10 +247,10 @@ module.exports = class Repository {
           }), 'Enabling Dependabot alerts')))
           return Promise.resolve(resArray)
         }
-        return this.github.repos.enableVulnerabilityAlerts({
+        return meteredPlugin(this, () => this.github.repos.enableVulnerabilityAlerts({
           owner: repoData.owner.login,
           repo: repoData.name
-        })
+        }))
       } else {
         this.log(`Disabling Dependabot alerts for for owner: ${repoData.owner.login} and repo ${repoData.name}`)
         if (this.nop) {
@@ -273,10 +260,10 @@ module.exports = class Repository {
           }), 'Disabling Dependabot alerts')))
           return Promise.resolve(resArray)
         }
-        return this.github.repos.disableVulnerabilityAlerts({
+        return meteredPlugin(this, () => this.github.repos.disableVulnerabilityAlerts({
           owner: repoData.owner.login,
           repo: repoData.name
-        })
+        }))
       }
     } else {
       this.log(`no need to update security for ${repoData.name}`)
@@ -298,10 +285,10 @@ module.exports = class Repository {
           }), 'Enabling Dependabot security updates')))
           return Promise.resolve(resArray)
         }
-        return this.github.repos.enableAutomatedSecurityFixes({
+        return meteredPlugin(this, () => this.github.repos.enableAutomatedSecurityFixes({
           owner: repoData.owner.login,
           repo: repoData.name
-        })
+        }))
       } else {
         this.log(`Disabling Dependabot security updates for owner: ${repoData.owner.login} and repo ${repoData.name}`)
         if (this.nop) {
@@ -311,10 +298,10 @@ module.exports = class Repository {
           }), 'Disabling Dependabot security updates')))
           return Promise.resolve(resArray)
         }
-        return this.github.repos.disableAutomatedSecurityFixes({
+        return meteredPlugin(this, () => this.github.repos.disableAutomatedSecurityFixes({
           owner: repoData.owner.login,
           repo: repoData.name
-        })
+        }))
       }
     }
   }

lib/plugins/teams.js

@@ -1,5 +1,6 @@
 const Diffable = require('./diffable')
 const NopCommand = require('../nopcommand')
+const { meteredPlugin } = require('../metrics')
 
 const teamRepoEndpoint = '/orgs/:owner/teams/:team_slug/repos/:owner/:repo'
 module.exports = class Teams extends Diffable {
@@ -44,7 +45,7 @@ module.exports = class Teams extends Diffable {
         new NopCommand(this.constructor.name, this.repo, this.github.request.endpoint(`PUT ${teamRepoEndpoint}`, this.toParams(existing, attrs)), 'Add Teams to Repo')
       ])
     }
-    return this.github.request(`PUT ${teamRepoEndpoint}`, this.toParams(existing, attrs))
+    return meteredPlugin(this, () => this.github.request(`PUT ${teamRepoEndpoint}`, this.toParams(existing, attrs)))
   }
 
   add (attrs) {
@@ -58,7 +59,7 @@ module.exports = class Teams extends Diffable {
           new NopCommand(this.constructor.name, this.repo, this.github.teams.addOrUpdateRepoPermissionsInOrg.endpoint(this.toParams(existing, attrs)), 'Add Teams to Repo')
         ])
       }
-      return this.github.teams.addOrUpdateRepoPermissionsInOrg(this.toParams(existing, attrs)).then(res => {
+      return meteredPlugin(this, () => this.github.teams.addOrUpdateRepoPermissionsInOrg(this.toParams(existing, attrs))).then(res => {
         this.log.debug(`team added ${res}`)
       }).catch(e => {
         this.log.error(`Error adding team to repo ${JSON.stringify(e)} with parms ${JSON.stringify(this.toParams(existing, attrs))}:\n`, e)
@@ -78,7 +79,7 @@ module.exports = class Teams extends Diffable {
             new NopCommand(this.constructor.name, this.repo, this.github.teams.create.endpoint(createParam), 'Create Team')
           ])
         }
-        return this.github.teams.create(createParam).then(res => {
+        return meteredPlugin(this, this.github.teams.create(createParam)).then(res => {
           this.log.debug(`team ${createParam.name} created`)
           existing = res.data
           this.log.debug(`adding team ${attrs.name} to repo ${this.repo.repo}`)
@@ -99,10 +100,10 @@ module.exports = class Teams extends Diffable {
         ), 'DELETE Team')
       ])
     }
-    return this.github.request(
+    return meteredPlugin(this, () => this.github.request(
       `DELETE ${teamRepoEndpoint}`,
       { team_slug: existing.slug, ...this.repo, org: this.repo.owner }
-    )
+    ))
   }
 
   toParams (existing, attrs) {