Update push gateway #966
Update push gateway #966
Conversation
nklincoln
force-pushed
the
update-push-gateway
branch
from
c8bc86b to
1317b4d
Compare
| const Constants = require('../../common/utils/constants'); | ||
|
|
||
| const prometheus = require('prom-client'); | ||
| const promGcStats = require('prometheus-gc-stats'); |
There was a problem hiding this comment.
expand prom to prometheus
| previouslySubmittedTotal: 0 | ||
| }; | ||
| const url = CaliperUtils.augmentUrlWithBasicAuth(options.pushUrl, Constants.AuthComponents.PushGateway); | ||
| this.prometheusClient = new prometheus.Pushgateway(url, null, this.registry); |
There was a problem hiding this comment.
is this a prometheus push gateway client ? might be worth naming the variable explicitly to say that
|
|
||
| const express = require('express'); | ||
| const appServer = express(); | ||
| const promClient = require('prom-client'); |
There was a problem hiding this comment.
would recommend expanding prom to prometheus
packages/caliper-core/package.json
Outdated
| "fs-extra": "8.1.0", | ||
| "js-yaml": "^3.13.1", | ||
| "mustache": "^2.3.0", | ||
| "mqtt": "3.0.0", | ||
| "nconf": "^0.10.0", | ||
| "nconf-yaml": "^1.0.2", | ||
| "pidusage": "^1.1.6", | ||
| "prom-client": "^11.5.3", |
| describe('caliper utilities', () => { | ||
|
|
||
|
|
||
| describe('#augmentUrlWithBasicAuth', () => { |
There was a problem hiding this comment.
would suggest providing a description rather than the name of the function
| constructor(options, messenger, workerIndex) { | ||
| super(messenger, workerIndex); | ||
| this.metricPath = options.metricPath || '/metrics'; | ||
| this.port = Number(options.port) || ConfigUtil.get(ConfigUtil.keys.Monitor.PrometheusScrapePort); |
There was a problem hiding this comment.
Is this an inbound port or outbound port ? would be good if the variable was a bit more descriptive
nklincoln
force-pushed
the
update-push-gateway
branch
from
1317b4d to
c834e00
Compare
| this.defaultLabels.roundLabel = this.roundLabel; | ||
| this.registry.setDefaultLabels(this.defaultLabels); | ||
|
|
||
| const self = this; |
There was a problem hiding this comment.
Is self necessary ? I thought javascript correctly handled it when using arrow functions
| */ | ||
| txFinished(results) { | ||
| if (Array.isArray(results)) { | ||
| for (let result of results) { |
| describe('caliper utilities', () => { | ||
|
|
||
|
|
||
| describe('Using the static method "augmentUrlWithBasicAuth" to augment base URLs with basic auth', () => { |
There was a problem hiding this comment.
would suggest either removing this describe altogether or changing it to
when augmenting the urls
We want to avoid putting implementation details into descriptions if possible and just describe the expected behaviour
There was a problem hiding this comment.
Will do, slowly adjusting to behavioural descriptions 😂
nklincoln
force-pushed
the
update-push-gateway
branch
from
c834e00 to
425899e
Compare
Contributes to #637
Modifies the prometheus-push-tx-observer to use prom-client and push the same metrics as available if using scrape mechanism:
User may provide options for:
Additionally, should a user provide runtime settings for basic auth, these may be used, with the following flags:
--caliper-auth-prometheuspush-username=bob--caliper-auth-prometheuspush-password=secretsIt may be of interest to add the basic auth capability to the prometheus monitor