Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add per-badge metrics for BaseService #3093

Merged
merged 14 commits into from
Feb 27, 2019
8 changes: 7 additions & 1 deletion core/base-service/base-non-memory-caching.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,15 @@ const { prepareRoute, namedParamsForMatch } = require('./route')
// configured by the service, the user's request, and the server's default
// cache length.
module.exports = class NonMemoryCachingBaseService extends BaseService {
static register({ camp }, serviceConfig) {
static register({ camp, requestCounter }, serviceConfig) {
const { cacheHeaders: cacheHeaderConfig } = serviceConfig
const { _cacheLength: serviceDefaultCacheLengthSeconds } = this
const { regex, captureNames } = prepareRoute(this.route)

const serviceRequestCounter = this._createServiceRequestCounter({
requestCounter,
})

camp.route(regex, async (queryParams, match, end, ask) => {
const namedParams = namedParamsForMatch(captureNames, match, this)
const serviceData = await this.invoke(
Expand Down Expand Up @@ -59,6 +63,8 @@ module.exports = class NonMemoryCachingBaseService extends BaseService {
})

makeSend(format, ask.res, end)(svg)

serviceRequestCounter.inc()
})
}
}
8 changes: 7 additions & 1 deletion core/base-service/base-static.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,16 @@ const coalesceBadge = require('./coalesce-badge')
const { prepareRoute, namedParamsForMatch } = require('./route')

module.exports = class BaseStaticService extends BaseService {
static register({ camp }, serviceConfig) {
static register({ camp, requestCounter }, serviceConfig) {
const {
profiling: { makeBadge: shouldProfileMakeBadge },
} = serviceConfig
const { regex, captureNames } = prepareRoute(this.route)

const serviceRequestCounter = this._createServiceRequestCounter({
requestCounter,
})

camp.route(regex, async (queryParams, match, end, ask) => {
analytics.noteRequest(queryParams, match)

Expand Down Expand Up @@ -58,6 +62,8 @@ module.exports = class BaseStaticService extends BaseService {
setCacheHeadersForStaticResource(ask.res)

makeSend(format, ask.res, end)(svg)

serviceRequestCounter.inc()
})
}
}
23 changes: 22 additions & 1 deletion core/base-service/base.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict'

const decamelize = require('decamelize')
// See available emoji at http://emoji.muan.co/
const emojic = require('emojic')
const Joi = require('joi')
Expand Down Expand Up @@ -317,11 +318,29 @@ module.exports = class BaseService {
return serviceData
}

static register({ camp, handleRequest, githubApiProvider }, serviceConfig) {
static _createServiceRequestCounter({ requestCounter }) {
if (requestCounter) {
const { category, serviceFamily, name } = this
const service = decamelize(name)
return requestCounter.labels(category, serviceFamily, service)
} else {
// When metrics are disabled, return a mock counter.
return { inc: () => {} }
}
}

static register(
{ camp, handleRequest, githubApiProvider, requestCounter },
serviceConfig
) {
const { cacheHeaders: cacheHeaderConfig, fetchLimitBytes } = serviceConfig
const { regex, captureNames } = prepareRoute(this.route)
const queryParams = getQueryParamNames(this.route)

const serviceRequestCounter = this._createServiceRequestCounter({
requestCounter,
})

camp.route(
regex,
handleRequest(cacheHeaderConfig, {
Expand All @@ -348,6 +367,8 @@ module.exports = class BaseService {
// The final capture group is the extension.
const format = match.slice(-1)[0]
sendBadge(format, badgeData)

serviceRequestCounter.inc()
},
cacheLength: this._cacheLength,
fetchLimitBytes,
Expand Down
1 change: 1 addition & 0 deletions core/base-service/categories.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,6 @@ function assertValidCategory(category, message = undefined) {
}

module.exports = {
isValidCategory,
assertValidCategory,
}
50 changes: 30 additions & 20 deletions core/base-service/deprecated-service.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,40 @@
'use strict'

const Joi = require('joi')
const camelcase = require('camelcase')
const BaseService = require('./base')
const { isValidCategory } = require('./categories')
const { Deprecated } = require('./errors')
const { isValidRoute } = require('./route')

const attrSchema = Joi.object({
route: isValidRoute,
name: Joi.string(),
label: Joi.string(),
category: isValidCategory,
// The content of examples is validated later, via `transformExamples()`.
examples: Joi.array().default([]),
message: Joi.string(),
dateAdded: Joi.date().required(),
}).required()

function deprecatedService(attrs) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda liked the old function signature with the named fields on the param object. Any reason we can't (or shouldn't) keep that? I guess that'd require a different argument on line 23 below (although I assume something like arguments[0] or something else could still make that work)

Not a blocking issue for me but wanted to ask

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to think of arguments as being magical and leading to unexpected things. Though here I guess the reason would be to allow using a more readable function signature with a concise call to Joi.validate. I'll do that if you think it improves things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah and that's a fair tradeoff. I just like it when my editor gives me good intellisense 😄 I don't feel particularly strongly either way and so defer to your preference. In fairness, this isn't something that will be used all that commonly (at least I hope there won't be a ton of deprecated services)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, I just went to update this, but ran into an issue where the variables also have to be re-assigned inside if we want to let Joi set default values. I feel like that makes it not worth it.

Related aside: I'm not sure there is an issue open for it yet, but autocomplete-friendly docs for core + helpers is on the roadmap. Maybe we should get that started! Do you know of any tooling for checking that, or what format should be used?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like that makes it not worth it.

SGTM! Doing one final glance over this and then will approve

Do you know of any tooling for checking that, or what format should be used?

Do you mean tooling to check for autocomplete friendliness? I suppose the answer is no either way 😆 but can start digging into it 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I guess it's to validate that the documentation is complete, formatted correctly, and matches the function.

Ooh, there's an eslint plugin!

https://github.com/gajus/eslint-plugin-jsdoc

We could turn it some of those on as warnings, and maybe some as errors, and start working our way through…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! We'd able to leverage the eslint overrides feature from the root config to set the rule for certain files/directories progressively to do just that

const { route, name, label, category, examples, message } = Joi.attempt(
attrs,
attrSchema,
`Deprecated service for ${attrs.route.base}`
)

// Only `url` is required.
function deprecatedService({
route,
label,
category,
examples = [],
message,
dateAdded,
}) {
return class DeprecatedService extends BaseService {
static get name() {
return (
name ||
`Deprecated${camelcase(route.base.replace(/\//g, '_'), {
pascalCase: true,
})}`
)
}

static get category() {
return category
}
Expand All @@ -26,17 +47,6 @@ function deprecatedService({
return true
}

static validateDefinition() {
super.validateDefinition()
Joi.assert(
{ dateAdded },
Joi.object({
dateAdded: Joi.date().required(),
}),
`Deprecated service for ${route.base}`
)
}

static get defaultBadgeData() {
return { label }
}
Expand Down
25 changes: 16 additions & 9 deletions core/base-service/deprecated-service.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,29 +5,36 @@ const deprecatedService = require('./deprecated-service')

describe('DeprecatedService', function() {
const route = {
base: 'coverity/ondemand',
base: 'service/that/no/longer/exists',
format: '(?:.+)',
}
const category = 'analysis'
const dateAdded = new Date()
const commonAttrs = { route, category, dateAdded }

it('returns true on isDeprecated', function() {
const service = deprecatedService({ route })
const service = deprecatedService({ ...commonAttrs })
expect(service.isDeprecated).to.be.true
})

it('has the expected name', function() {
const service = deprecatedService({ ...commonAttrs })
expect(service.name).to.equal('DeprecatedServiceThatNoLongerExists')
})

it('sets specified route', function() {
const service = deprecatedService({ route })
const service = deprecatedService({ ...commonAttrs })
expect(service.route).to.deep.equal(route)
})

it('sets specified label', function() {
const label = 'coverity'
const service = deprecatedService({ route, label })
const service = deprecatedService({ ...commonAttrs, label })
expect(service.defaultBadgeData.label).to.equal(label)
})

it('sets specified category', function() {
const category = 'analysis'
const service = deprecatedService({ route, category })
const service = deprecatedService({ ...commonAttrs })
expect(service.category).to.equal(category)
})

Expand All @@ -37,12 +44,12 @@ describe('DeprecatedService', function() {
title: 'Not sure we would have examples',
},
]
const service = deprecatedService({ route, examples })
const service = deprecatedService({ ...commonAttrs, examples })
expect(service.examples).to.deep.equal(examples)
})

it('uses default deprecation message when no message specified', async function() {
const service = deprecatedService({ route })
const service = deprecatedService({ ...commonAttrs })
expect(await service.invoke()).to.deep.equal({
isError: true,
color: 'lightgray',
Expand All @@ -52,7 +59,7 @@ describe('DeprecatedService', function() {

it('uses custom deprecation message when specified', async function() {
const message = 'extended outage'
const service = deprecatedService({ route, message })
const service = deprecatedService({ ...commonAttrs, message })
expect(await service.invoke()).to.deep.equal({
isError: true,
color: 'lightgray',
Expand Down
48 changes: 40 additions & 8 deletions core/base-service/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const path = require('path')
const glob = require('glob')
const countBy = require('lodash.countby')
const categories = require('../../services/categories')
const BaseService = require('./base')
const { assertValidServiceDefinitionExport } = require('./service-definitions')
Expand All @@ -20,42 +21,72 @@ function loadServiceClasses(servicePaths) {
servicePaths = glob.sync(path.join(serviceDir, '**', '*.service.js'))
}

const serviceClasses = []
servicePaths.forEach(path => {
const module = require(path)
let serviceClasses = []
servicePaths.forEach(servicePath => {
const module = require(servicePath)

const theseServiceClasses = []
if (
!module ||
(module.constructor === Array && module.length === 0) ||
(module.constructor === Object && Object.keys(module).length === 0)
) {
throw new InvalidService(
`Expected ${path} to export a service or a collection of services`
`Expected ${servicePath} to export a service or a collection of services`
)
} else if (module.prototype instanceof BaseService) {
serviceClasses.push(module)
theseServiceClasses.push(module)
} else if (module.constructor === Array || module.constructor === Object) {
for (const key in module) {
const serviceClass = module[key]
if (serviceClass.prototype instanceof BaseService) {
serviceClasses.push(serviceClass)
theseServiceClasses.push(serviceClass)
} else {
throw new InvalidService(
`Expected ${path} to export a service or a collection of services; one of them was ${serviceClass}`
`Expected ${servicePath} to export a service or a collection of services; one of them was ${serviceClass}`
)
}
}
} else {
throw new InvalidService(
`Expected ${path} to export a service or a collection of services; got ${module}`
`Expected ${servicePath} to export a service or a collection of services; got ${module}`
)
}

// Decorate each service class with the directory that contains it.
theseServiceClasses.forEach(serviceClass => {
serviceClass.serviceFamily = servicePath
.replace(serviceDir, '')
.split(path.sep)[1]
})

serviceClasses = serviceClasses.concat(theseServiceClasses)
})

serviceClasses.forEach(ServiceClass => ServiceClass.validateDefinition())

return serviceClasses
}

function assertNamesUnique(names, { message }) {
const duplicates = {}
Object.entries(countBy(names))
.filter(([name, count]) => count > 1)
.forEach(([name, count]) => {
duplicates[name] = count
})
if (Object.keys(duplicates).length) {
throw new Error(`${message}: ${JSON.stringify(duplicates, undefined, 2)}`)
}
}

function checkNames() {
const services = loadServiceClasses()
assertNamesUnique(services.map(({ name }) => name), {
message: 'Duplicate service names found',
})
}

function collectDefinitions() {
const services = loadServiceClasses()
// flatMap.
Expand All @@ -78,6 +109,7 @@ function loadTesters() {
module.exports = {
InvalidService,
loadServiceClasses,
checkNames,
collectDefinitions,
loadTesters,
}
Loading