feature: update prom-client #123

PixnBits · 2020-04-28T22:56:33Z

Description

prom-client 12.x added more metrics which cover those that we added via prometheus-gc-stats. Additionally, prom-client is making use of the new perf_hooks API in Node.JS removing the need for gc-stats (binary dep). This, like #45 will help reduce potential install problems and binary incompatibilities (ex: switching base docker images).

Motivation and Context

This keeps One App up-to-date and reduces installation complexity.

How Has This Been Tested?

Integration tests have been added showing the metric names exposed.

Types of Changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation (adding or updating documentation)

Probably a breaking change as the metric names change (and reducing names but increasing labels for some of that information). I'm hoping this can be added before v5.0.0 is officially released.

Checklist:

My change requires a change to the documentation and I have updated the documentation accordingly.
These changes should be applied to a maintenance branch.
This change requires cross browser checks.
Performance tests should be ran against the server prior to merging.
This change impacts caching for client browsers.
This change impacts HTTP headers.
This change adds additional environment variable requirements for One App users.
I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using One App?

This reduces one of the two native dependencies and so hopefully makes installation easier.

PixnBits · 2020-04-28T22:58:24Z

__tests__/integration/__snapshots__/one-app.spec.js.snap

@@ -42,6 +42,44 @@ Object {
 }
 `;

+exports[`Tests that require Docker setup one-app successfully started metrics has all metrics 1`] = `


I'd encourage looking at the diff of this file across the two commits to see the difference in metric names

oneamexbot · 2020-04-28T23:00:44Z

📊 Bundle Size Report

file name	size on disk	gzip
app.js	113.2KB	31.6KB
runtime.js	15KB	5.4KB
vendors.js	128.4KB	38KB
app~vendors.js	433.9KB	112.5KB
service-worker-client.js	16.9KB	5.2KB
legacy/app.js	120KB	33.2KB
legacy/runtime.js	15KB	5.4KB
legacy/vendors.js	163.4KB	44.9KB
legacy/app~vendors.js	442.2KB	114.7KB
legacy/service-worker-client.js	17.4KB	5.4KB

Generated by 🚫 dangerJS against 698e5b1

PixnBits · 2020-04-28T23:15:12Z

__tests__/integration/__snapshots__/one-app.spec.js.snap

+  "nodejs_eventloop_lag_min_seconds",
+  "nodejs_eventloop_lag_p50_seconds",
+  "nodejs_eventloop_lag_p90_seconds",
+  "nodejs_eventloop_lag_p99_seconds"


from Travis:

- "nodejs_eventloop_lag_p99_seconds" + "nodejs_eventloop_lag_p99_seconds",

🤦

as there aren't any reviews yet and we might want to keep the two commits separate I've replaced 39b35b5 with 3b23458 to reduce the need for squashing later
(might be moot if the metric test isn't desired on it's own, but just in case)

JamesSingleton · 2020-04-30T23:49:49Z

prod-sample/docker-compose.yml

@@ -14,6 +14,7 @@ services:
    image: one-app:at-test
    expose:
      - "8443"
+      - "3005"


I know it is the sample, but what expose 3005 now?

infoxicator · 2020-05-05T10:55:19Z

__tests__/integration/helpers/testRunner.js

@@ -46,7 +46,8 @@ const setUpTestRunner = async ({ oneAppLocalPortToUse } = {}) => {
          'one-app': {
            ports: [
              `${oneAppLocalPortToUse}:8443`,
-            ],
+              oneAppMetricsLocalPortToUse ? `${oneAppMetricsLocalPortToUse}:3005` : undefined,


@JamesSingleton the 3005 port has to be exposed because it is mapped here like from ramdomPort to 3005

__tests__/integration/one-app.spec.js

also removes the need for gc-stats, using the Node.JS API instead

PixnBits · 2020-05-14T01:05:45Z

    Snapshot name: `Tests that require Docker setup one-app successfully started metrics has all metrics 1`

    - Snapshot
    + Received

    @@ -1,6 +1,8 @@
      Array [
    +   "circuit",
    +   "circuit_perf",

ah, #111 was merged! I ran the tests before rebasing but not after 🤦‍♂️

infoxicator · 2020-05-14T11:41:34Z

    Snapshot name: `Tests that require Docker setup one-app successfully started metrics has all metrics 1`

    - Snapshot
    + Received

    @@ -1,6 +1,8 @@
      Array [
    +   "circuit",
    +   "circuit_perf",

ah, #111 was merged! I ran the tests before rebasing but not after 🤦‍♂️

I thought I updated it but, might have messed up because I did it from my fork

PixnBits requested review from a team as code owners April 28, 2020 22:56

PixnBits commented Apr 28, 2020

View reviewed changes

PixnBits force-pushed the feature/update-prom-client branch from 3b23458 to 39b35b5 Compare April 28, 2020 23:23

10xLaCroixDrinker previously approved these changes Apr 29, 2020

View reviewed changes

JamesSingleton reviewed Apr 30, 2020

View reviewed changes

infoxicator reviewed May 5, 2020

View reviewed changes

infoxicator previously approved these changes May 5, 2020

View reviewed changes

infoxicator dismissed stale reviews from 10xLaCroixDrinker and themself via 8006fdc May 5, 2020 12:29

infoxicator previously approved these changes May 5, 2020

View reviewed changes

mtomcal suggested changes May 5, 2020

View reviewed changes

__tests__/integration/one-app.spec.js Outdated Show resolved Hide resolved

PixnBits dismissed infoxicator’s stale review via b4f779d May 13, 2020 23:43

PixnBits force-pushed the feature/update-prom-client branch from 4671577 to b4f779d Compare May 13, 2020 23:43

mtomcal previously approved these changes May 13, 2020

View reviewed changes

PixnBits added 3 commits May 13, 2020 17:58

test(integration): record metric names and ensure descriptions

9364b8b

feat(metrics): update prom-client for more dimensions

d133259

also removes the need for gc-stats, using the Node.JS API instead

test(integration): use a positive scenario for metric help

c6d49a1

PixnBits dismissed mtomcal’s stale review via c6d49a1 May 14, 2020 02:02

PixnBits force-pushed the feature/update-prom-client branch from b4f779d to c6d49a1 Compare May 14, 2020 02:02

infoxicator approved these changes May 14, 2020

View reviewed changes

Merge branch 'master' into feature/update-prom-client

91633a4

mtomcal approved these changes May 14, 2020

View reviewed changes

Francois-Esquire approved these changes May 14, 2020

View reviewed changes

Francois-Esquire added 2 commits May 14, 2020 18:04

Merge branch 'master' into feature/update-prom-client

595c271

Merge branch 'master' into feature/update-prom-client

34de45e

Francois-Esquire added 2 commits May 21, 2020 15:23

Merge branch 'master' into feature/update-prom-client

5b5b6fc

Merge branch 'master' into feature/update-prom-client

698e5b1

infoxicator merged commit 79f0e68 into master May 22, 2020

infoxicator deleted the feature/update-prom-client branch May 22, 2020 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: update prom-client #123

feature: update prom-client #123

PixnBits commented Apr 28, 2020 •

edited

Loading

PixnBits Apr 28, 2020 •

edited

Loading

oneamexbot commented Apr 28, 2020 •

edited

Loading

PixnBits Apr 28, 2020

PixnBits Apr 28, 2020

JamesSingleton Apr 30, 2020

infoxicator May 5, 2020

PixnBits commented May 14, 2020

infoxicator commented May 14, 2020

feature: update prom-client #123

feature: update prom-client #123

Conversation

PixnBits commented Apr 28, 2020 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of Changes

Checklist:

What is the Impact to Developers Using One App?

PixnBits Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

oneamexbot commented Apr 28, 2020 • edited Loading

📊 Bundle Size Report

PixnBits Apr 28, 2020

Choose a reason for hiding this comment

PixnBits Apr 28, 2020

Choose a reason for hiding this comment

JamesSingleton Apr 30, 2020

Choose a reason for hiding this comment

infoxicator May 5, 2020

Choose a reason for hiding this comment

PixnBits commented May 14, 2020

infoxicator commented May 14, 2020

PixnBits commented Apr 28, 2020 •

edited

Loading

PixnBits Apr 28, 2020 •

edited

Loading

oneamexbot commented Apr 28, 2020 •

edited

Loading