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

Cd/add metrics logger #11

Merged
merged 19 commits into from Jan 16, 2019
Merged

Cd/add metrics logger #11

merged 19 commits into from Jan 16, 2019

Conversation

chribsen
Copy link
Contributor

No description provided.

Christian de Fries and others added 6 commits January 11, 2019 10:59
Module that keeps maintains a global singleton variable to retain
metric state and log them on demand.

References: connectedcars/api#854
Global singletons are close to impossible to mock properly.
concats name and labels into a key
Function getPrometheusMetrics assumed that metrics.labels was
always set. This commit makes a precheck to handle the case of no
labels.
@coveralls
Copy link

coveralls commented Jan 16, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 47aac22 on cd/add-metrics-logger into ce8fa1e on master.

@chribsen
Copy link
Contributor Author

@mathiastj do we have any node 6 projects that we should be compatible with?

@mathiastj
Copy link
Contributor

I can't find any, so I think we're good.

@mathiastj
Copy link
Contributor

mathiastj commented Jan 16, 2019

Should probably update the README to show how to use the new format.
Also create a test that checks that it logs to the console what we expect it to. This is also needed to bring coverage back to 100% (use nyc locally to figure out which lines are missing tests)

chribsen added 7 commits January 16, 2019 10:41
Keys did not include the label name, thus causing possible
colissions. Also, logMetrics was iterating the values instead of
the keys.
The default switch can never be reached. Only if the API consumer
is fiddling around with this.metrics and adds bogus keys and
values.
startTime is not used on gauge type. Only cumulative type.
Also adapts existing tests to cover the changes in the prev.
commits
@chribsen
Copy link
Contributor Author

Just talked to @mathiastj : It probably makes sense to make gauge() log the data point immediately as it represents a given point in time

chribsen added 2 commits January 16, 2019 14:40
the `gauge()` function now takes an extra argument, thus making
it possible to do other than takes the last value
const key = this.createKey(name, labels)
const metric = this.metrics[key]
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to be consistent, i.e. in the cumulative function too

chribsen added 2 commits January 16, 2019 15:09
Eventually the timestamps will end up on Google Custom Metrics
and they expect ISO-8601 timestamps
@chribsen chribsen merged commit a36a2e4 into master Jan 16, 2019
@chribsen chribsen deleted the cd/add-metrics-logger branch January 16, 2019 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants