use global metric registry #128

finkr · 2019-07-24T23:40:14Z

Use the global REGISTRY, in order :

add exporter runtime metrics (fix add exporter runtime metrics #9 )
add more metrics later (collection time for each object type)

This patch is intended to expose internal metrics on /metrics only for the default section (so people with multiple sections ?section=something don't get duplicate metrics).

Use the global REGISTRY, in order : 1) add exporter runtime metrics (fix pryorda#9 ) 2) add more metrics later (collection time for each object type) This patch is intended to expose internal metrics on `/metrics` only for the default section (so people with multiple sections `?section=something` don't get duplicate metrics).

pryorda · 2019-07-26T06:49:55Z

First off, Thanks for your contributions. Is there any way you could add tests for this?

pryorda

LGTM, Just need tests?

codecov-io · 2019-07-27T00:06:11Z

Codecov Report

Merging #128 (8912ac8) into master (8cb2284) will increase coverage by 93.62%.
The diff coverage is 75.00%.

@@             Coverage Diff             @@
##           master     #128       +/-   ##
===========================================
+ Coverage        0   93.62%   +93.62%     
===========================================
  Files           0        4        +4     
  Lines           0      549      +549     
===========================================
+ Hits            0      514      +514     
- Misses          0       35       +35

Impacted Files	Coverage Δ
vmware_exporter/vmware_exporter.py	`92.84% <75.00%> (ø)`
vmware_exporter/__init__.py	`100.00% <0.00%> (ø)`
vmware_exporter/defer.py	`97.22% <0.00%> (ø)`
vmware_exporter/helpers.py	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cb2284...1b5af5e. Read the comment docs.

finkr · 2019-07-27T00:07:16Z

@pryorda ,

The prometheus client library adds many metrics (plus process metrics on Linux).

I was about to create a unit test to check python_info only (value and labels).
Also, I was considering to a a test to ensure those metrics are published on the default section only.

Do you think we need more tests ?

# HELP python_gc_objects_collected_total Objects collected during gc
# TYPE python_gc_objects_collected_total counter
python_gc_objects_collected_total{generation="0"} 633.0
python_gc_objects_collected_total{generation="1"} 301.0
python_gc_objects_collected_total{generation="2"} 0.0
# HELP python_gc_objects_uncollectable_total Uncollectable object found during GC
# TYPE python_gc_objects_uncollectable_total counter
python_gc_objects_uncollectable_total{generation="0"} 0.0
python_gc_objects_uncollectable_total{generation="1"} 0.0
python_gc_objects_uncollectable_total{generation="2"} 0.0
# HELP python_gc_collections_total Number of times this generation was collected
# TYPE python_gc_collections_total counter
python_gc_collections_total{generation="0"} 191.0
python_gc_collections_total{generation="1"} 17.0
python_gc_collections_total{generation="2"} 1.0
# HELP python_info Python platform information
# TYPE python_info gauge
python_info{implementation="CPython",major="3",minor="7",patchlevel="2",version="3.7.2"} 1.0

pryorda · 2019-07-27T02:10:46Z

That should be fine. Sent from ProtonMail mobile

…

-------- Original Message --------

On Jul 26, 2019, 6:07 PM, Frank lin Piat wrote: ***@***.***(https://github.com/pryorda) , The prometheus client library adds many metrics (plus process metrics on Linux). I was about to create a unit test to check python_info only (value and labels). Also, I was considering to a a test to ensure those metrics are published on the default section only. Do you think we need more tests ? # HELP python_gc_objects_collected_total Objects collected during gc # TYPE python_gc_objects_collected_total counter python_gc_objects_collected_total{generation="0"} 633.0 python_gc_objects_collected_total{generation="1"} 301.0 python_gc_objects_collected_total{generation="2"} 0.0 # HELP python_gc_objects_uncollectable_total Uncollectable object found during GC # TYPE python_gc_objects_uncollectable_total counter python_gc_objects_uncollectable_total{generation="0"} 0.0 python_gc_objects_uncollectable_total{generation="1"} 0.0 python_gc_objects_uncollectable_total{generation="2"} 0.0 # HELP python_gc_collections_total Number of times this generation was collected # TYPE python_gc_collections_total counter python_gc_collections_total{generation="0"} 191.0 python_gc_collections_total{generation="1"} 17.0 python_gc_collections_total{generation="2"} 1.0 # HELP python_info Python platform information # TYPE python_info gauge python_info{implementation="CPython",major="3",minor="7",patchlevel="2",version="3.7.2"} 1.0 — You are receiving this because you were mentioned. Reply to this email directly, [view it on GitHub](#128?email_source=notifications&email_token=ABLCAHFBA6FGBU6ELSLNRDTQBOGTLA5CNFSM4IGVUDC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD257AXI#issuecomment-515633245), or [mute the thread](https://github.com/notifications/unsubscribe-auth/ABLCAHFPZ4P7QTINMVW2PP3QBOGTLANCNFSM4IGVUDCQ).

codecov-commenter · 2021-07-19T13:07:06Z

Codecov Report

Merging #128 (a509e07) into main (802240c) will increase coverage by 1.34%.
The diff coverage is 86.04%.

@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
+ Coverage   80.29%   81.64%   +1.34%     
==========================================
  Files           4        4              
  Lines         873      888      +15     
==========================================
+ Hits          701      725      +24     
+ Misses        172      163       -9

Impacted Files	Coverage Δ
vmware_exporter/defer.py	`97.22% <ø> (ø)`
vmware_exporter/helpers.py	`72.72% <20.00%> (+11.53%)`	⬆️
vmware_exporter/vmware_exporter.py	`81.48% <94.59%> (+0.39%)`	⬆️
vmware_exporter/__init__.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a202941...a509e07. Read the comment docs.

and declare metrics vmware_exporter_build_info{version="..."}

use exporter_metrics in configuration file

E302 expected 2 blank lines

finkr · 2021-07-23T15:33:43Z

Hello Daniel,

Does this look good to you @pryorda ?

pryorda · 2021-07-24T23:51:48Z

Hey @finkr,

Appreciate the work on this. Ill work on getting it tested and updated.

Thanks for your patience on this

finkr · 2021-07-25T07:15:22Z

Hello Daniel,

I looked at other issues and pull request (making the default section optional).

Also the exporters should always return a page with the exporters metrics , and avoid http/500 errors with empty page. That's not easy to do with a single page with both VMware metrics and the exporters metrics

I am seriously considering to move the exporters own metric to a separate page .

This means users would have to configure two or more targets in Prometheus.

pryorda · 2021-08-05T06:10:34Z

Sorry for the delay. What would happen if you hit the endpoint multiple times quickly?

finkr · 2021-08-05T13:22:44Z

Sorry for the delay. What would happen if you hit the endpoint multiple times quickly?

@pryorda , Handling a single prometheus registry containing both vmware metrics AND the the exporter own metrics proves to be quite challenging, especially when it comes to ensure that we provide technical metrics even when an exception is raised when scraping the vCenter.

I have some work going on to split the exporter's own metrics in a separate endpoint (which means users will have to configure two prometheus jobs. And they may forget to do so unfortunately)

I'll try to submit that work soon.

pryorda · 2021-08-19T04:56:02Z

Sorry for the delay. I'm def not a fan of moving to two endpoints. Do you know if there is a best practice for service metrics vs vmware metrics?

finkr · 2021-08-19T20:37:38Z

For those exporters that are designed to expose metrics from multiple targets, it seems common to have 1 endpoint to expose the exporter own metrics, and another endpoints for targets.

Exporter name	own metrics	target metrics
snmp_exporter	/metrics	/snmp
blackbox_exporter	/metrics	/probe
json_exporter	/metrics	/probe
influxdb_exporter	/metrics/exporter	/metrics
openstack-exporter	/metrics	/probe
modbus exporter	/metrics	/modbus

Some exceptions I fount, where all metrics are exposed at once : statsd_exporter, pushgateway,

ipmi_exporter is a bit different because local ipmi interface are exposed under /metrics (along with the exporter own metrics), whereas remote ipmi target are exposed as /impi?target=

pryorda previously approved these changes Jul 26, 2019

View reviewed changes

fix /metrics matching

9c03820

finkr dismissed pryorda’s stale review via 9c03820 July 27, 2019 00:02

pryorda added 2 commits November 20, 2019 12:53

Merge branch 'master' into finkr-registry

854b667

Merge branch 'master' into finkr-registry

27ec6b5

pryorda requested review from dannyk81 and Jc2k December 1, 2019 04:49

pryorda added 2 commits November 30, 2019 21:49

Merge branch 'master' into finkr-registry

8912ac8

Merge branch 'master' into finkr-registry

1b5af5e

Base automatically changed from master to main March 9, 2021 18:47

Merge branch 'pryorda:main' into finkr-registry

4df27fd

Frank Lin Piat and others added 6 commits July 20, 2021 00:08

avoid re-registering metrics on 2nd run

106f174

and declare metrics vmware_exporter_build_info{version="..."}

Exporting exporter metrics is configurable

8c49faf

use exporter_metrics in configuration file

unit test for the export prometheus_client metrics

6185d60

document exporter monitoring in README

0db3024

fix flake8 warnings

53bdbd4

fix old flake8 warning

a509e07

E302 expected 2 blank lines

This was referenced Jul 25, 2021

updating prometheus_client ? #281

Open

memory leak when vcenter not reachable #282

Open

finkr and others added 2 commits August 20, 2021 18:53

Merge branch 'pryorda:main' into finkr-registry

44d2d4e

Merge branch 'main' into finkr-registry

47410a4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use global metric registry #128

use global metric registry #128

finkr commented Jul 24, 2019

pryorda commented Jul 26, 2019

pryorda left a comment

codecov-io commented Jul 27, 2019 •

edited

Loading

finkr commented Jul 27, 2019

pryorda commented Jul 27, 2019 via email

codecov-commenter commented Jul 19, 2021 •

edited

Loading

finkr commented Jul 23, 2021

pryorda commented Jul 24, 2021

finkr commented Jul 25, 2021

pryorda commented Aug 5, 2021

finkr commented Aug 5, 2021

pryorda commented Aug 19, 2021

finkr commented Aug 19, 2021

use global metric registry #128

Are you sure you want to change the base?

use global metric registry #128

Conversation

finkr commented Jul 24, 2019

pryorda commented Jul 26, 2019

pryorda left a comment

Choose a reason for hiding this comment

codecov-io commented Jul 27, 2019 • edited Loading

Codecov Report

finkr commented Jul 27, 2019

pryorda commented Jul 27, 2019 via email

codecov-commenter commented Jul 19, 2021 • edited Loading

Codecov Report

finkr commented Jul 23, 2021

pryorda commented Jul 24, 2021

finkr commented Jul 25, 2021

pryorda commented Aug 5, 2021

finkr commented Aug 5, 2021

pryorda commented Aug 19, 2021

finkr commented Aug 19, 2021

codecov-io commented Jul 27, 2019 •

edited

Loading

codecov-commenter commented Jul 19, 2021 •

edited

Loading