Enhancements to Prometheus exporter #609

NajmudheenCT · 2021-06-15T14:53:37Z

What this PR does / why we need it:

Support for new metrics
Fix for [v1.2.2 Testing] Gaps in Prometheus and Grafana graphs when Prometheus exporter is enabled #533
support for timestamp conversion based on prometheus server timezone
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes [v1.2.2 Testing] Gaps in Prometheus and Grafana graphs when Prometheus exporter is enabled #533

Special notes for your reviewer:
Change set from PR #595 and #588 are included in this
Release note:

Test Report
Prometheus graph displays all metrics for one port with properties
{resource_type="port", resource_id="port0", storage_sn="6013136b-f5ee-4acf-9146-d20e927f090b" }

…orage

joseph-v · 2021-06-16T05:20:53Z

delfin/exporter/prometheus/prometheus.py


        for timestamp, value in values.items():
-            f.write("storage_%s{%s} %f %d\n" % (metric, labels,
-                                                value, timestamp))
+            timestamp += self.timestamp_offset_ms


Timestamp update can be moved out of _write_to_prometheus_format() function.

values dictionary contains multiple timestamps.. each timestamp is distinct .. so adding offset has to be inside this loop

joseph-v · 2021-06-16T05:31:29Z

delfin/exporter/prometheus/prometheus.py


 class PrometheusExporter(object):

+    def __init__(self):
+        self.timestamp_offset_ms = self.set_timestamp_offset_from_utc_ms()


Using timestamp offset may result in incorrect results in some cases. (Eg. Day Light Saving changes in some regions)

Prometheus uses Unix time internally to persist metrics. for display Prometheus support timezone in display layer .. https://prometheus.io/docs/introduction/faq/

After initializing PrometheusExporter class, if day light saving changes, offset become wrong.

hmm right.. Actually Prometheus do not suggest timestamp from the exporter. This could be one reason. But on our case we have to pass the timestamp as we export historic values. Could not see any way to determine daylight saving. We will have to document that behavior.

i remember python time libraray has functions to determine if DST

If we do not cache offset, and get it just before export, it will be wrong just one time when day light savings change

done.. we get timezone info before every export now

codecov · 2021-06-16T06:29:57Z

Codecov Report

Merging #609 (28378d7) into master (ee87076) will decrease coverage by 0.06%.
The diff coverage is 39.39%.

@@            Coverage Diff             @@
##           master     #609      +/-   ##
==========================================
- Coverage   71.95%   71.88%   -0.07%     
==========================================
  Files         141      141              
  Lines       12492    12562      +70     
  Branches     1493     1499       +6     
==========================================
+ Hits         8988     9030      +42     
- Misses       2989     3017      +28     
  Partials      515      515

Impacted Files	Coverage Δ
delfin/exporter/prometheus/exporter_server.py	`0.00% <0.00%> (ø)`
delfin/exporter/prometheus/prometheus.py	`26.08% <24.07%> (-5.92%)`	⬇️
delfin/drivers/fake_storage/__init__.py	`95.14% <100.00%> (+8.64%)`	⬆️
delfin/drivers/api.py	`88.09% <0.00%> (+2.38%)`	⬆️

kumarashit · 2021-06-16T07:49:34Z

delfin/exporter/prometheus/exporter_server.py

-    cfg.StrOpt('metrics_cache_file', default='/var/lib/delfin/delfin_exporter'
-                                             '.txt',
-               help='The temp cache file used for persisting metrics'),
+    cfg.StrOpt('metrics_dir', default='/var/lib/delfin/metrics',


Can the dir name be CONSTANT driven?

+1 including folder and file names could be that way

Done, Thanks

kumarashit · 2021-06-16T10:20:58Z

delfin/exporter/prometheus/exporter_server.py

+    for file in glob.glob("*.prom"):
+        file_list.append(file)
+    data = ''
+    for file in file_list:


this loop can be merged into loop at 49, right?

kumarashit · 2021-06-16T10:23:39Z

delfin/exporter/prometheus/prometheus.py


 class PrometheusExporter(object):

+    def __init__(self):
+        self.timestamp_offset_ms = self.set_timestamp_offset_from_utc_ms()


i remember python time libraray has functions to determine if DST

skdwriting · 2021-06-17T10:19:45Z

delfin/api/schemas/storage_capabilities_schema.py

@@ -76,7 +76,7 @@
                                                'maxLength': 255}
                            },
                        },
-                        'writeRequests': {
+                        'writeIops': {


Is these metric names are common metric names? I see it is changed now. What is the possibility of further change? Can we use any key mapping to keep the code more maintainable?

applicable for all

Yes These are common metric names, based on our analysis on different platforms we derived common names now. hence to align we have changed this metric names. This is schema would be validated against driver capabilities.. drivers would use their mapping to map storage metric names with delfin metric names

skdwriting · 2021-06-17T10:24:29Z

delfin/drivers/fake_storage/__init__.py

-            for i in range(MIN_PERF_VALUES, MAX_PERF_VALUES):
-                timestamp = int(float(datetime.datetime.now().timestamp()
-                                      ) * 1000)
+            timestamp = start_time


is it system time?!/regional specific?

how is this used?

This is timestamp equivalent to UTC. since we support historic collection , this is the fake driver side emulation to support collection from provided start_time to end_time

skdwriting · 2021-06-17T10:26:12Z

delfin/exporter/prometheus/exporter_server.py

-    cfg.StrOpt('metrics_cache_file', default='/var/lib/delfin/delfin_exporter'
-                                             '.txt',
-               help='The temp cache file used for persisting metrics'),
+    cfg.StrOpt('metrics_dir', default='/var/lib/delfin/metrics',


+1 including folder and file names could be that way

delfin/exporter/prometheus/exporter_server.py

…o fix_exporter_issue

sushanthakumar · 2021-06-18T15:23:28Z

delfin/exporter/prometheus/prometheus.py


+grp = cfg.OptGroup('PROMETHEUS_EXPORTER')
+METRICS_CACHE_DIR = '/var/lib/delfin/metrics'


Should this be defined seperately in both exporter server and promethus files? Currenlty in it is in 2 places

yes because both code run on different processes .

sushanthakumar · 2021-06-18T15:26:55Z

delfin/exporter/prometheus/prometheus.py


    def push_to_prometheus(self, storage_metrics):
-        with open(cfg.CONF.PROMETHEUS_EXPORTER.metrics_cache_file, "a+") as f:
+        self.timestamp_offset_ms = self.set_timestamp_offset_from_utc_ms()


In error case, set_timestamp_offset_from_utc_ms returns 0, will that be fine from promethues side?if not we can handle this and next statements as errors and return

if we return error this collection will be lost.. if we set 0 atleast there is a chance that prometheus wil receive it and accepets/rejects based on its timezone and rejection time window..

kumarashit

LGTM

sushanthakumar · 2021-06-19T03:39:21Z

LGTM

joseph-v

LGTM

skdwriting

LGTM

NajmudheenCT added 3 commits June 3, 2021 15:20

updating common metric spec

839e31b

Adding implementaion for resource leval metric collection for fake st…

6062f77

…orage

Enhancements to prometheus exporter to support new metrics

b3f9fd3

joseph-v reviewed Jun 16, 2021

View reviewed changes

modify indentation

5bae681

NajmudheenCT force-pushed the fix_exporter_issue branch from d2df3d6 to 5bae681 Compare June 16, 2021 06:27

correcting labels overwritten probelm

97dd69e

kumarashit reviewed Jun 16, 2021

View reviewed changes

Adressing review comments

8ebb86b

skdwriting reviewed Jun 17, 2021

View reviewed changes

adding UTs for prometheus exporter

d68f2e9

kumarashit reviewed Jun 18, 2021

View reviewed changes

delfin/exporter/prometheus/exporter_server.py Show resolved Hide resolved

NajmudheenCT added 2 commits June 18, 2021 13:49

Merge branch 'master' of https://github.com/sodafoundation/delfin int…

8771404

…o fix_exporter_issue

using os.join for metrics file path

3e00bf7

NajmudheenCT force-pushed the fix_exporter_issue branch from e67d90f to 3e00bf7 Compare June 18, 2021 08:42

NajmudheenCT added 2 commits June 18, 2021 18:26

Adding code to check metrics dir exists

5db0b48

Adding code to check metrics dir exists

6a9bbba

NajmudheenCT force-pushed the fix_exporter_issue branch from 69a2982 to 6a9bbba Compare June 18, 2021 13:14

Merge branch 'master' into fix_exporter_issue

28378d7

sushanthakumar reviewed Jun 18, 2021

View reviewed changes

kumarashit approved these changes Jun 18, 2021

View reviewed changes

joseph-v approved these changes Jun 20, 2021

View reviewed changes

skdwriting approved these changes Jun 21, 2021

View reviewed changes

skdwriting merged commit 985bd0e into sodafoundation:master Jun 21, 2021

NajmudheenCT mentioned this pull request Jun 21, 2021

leader election framework for performance metric collection #589

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancements to Prometheus exporter #609

Enhancements to Prometheus exporter #609

NajmudheenCT commented Jun 15, 2021 •

edited

Loading

joseph-v Jun 16, 2021

NajmudheenCT Jun 16, 2021

joseph-v Jun 16, 2021

NajmudheenCT Jun 16, 2021 •

edited

Loading

joseph-v Jun 16, 2021

NajmudheenCT Jun 16, 2021

kumarashit Jun 16, 2021

joseph-v Jun 16, 2021

NajmudheenCT Jun 17, 2021

codecov bot commented Jun 16, 2021 •

edited

Loading

kumarashit Jun 16, 2021

skdwriting Jun 17, 2021

NajmudheenCT Jun 17, 2021

kumarashit Jun 16, 2021

NajmudheenCT Jun 17, 2021

kumarashit Jun 16, 2021

skdwriting Jun 17, 2021

skdwriting Jun 17, 2021

NajmudheenCT Jun 17, 2021

skdwriting Jun 17, 2021

skdwriting Jun 17, 2021

NajmudheenCT Jun 17, 2021

skdwriting Jun 17, 2021

sushanthakumar Jun 18, 2021 •

edited

Loading

NajmudheenCT Jun 19, 2021

sushanthakumar Jun 18, 2021

NajmudheenCT Jun 19, 2021

kumarashit left a comment

sushanthakumar commented Jun 19, 2021

joseph-v left a comment

skdwriting left a comment


		grp = cfg.OptGroup('PROMETHEUS_EXPORTER')
		METRICS_CACHE_DIR = '/var/lib/delfin/metrics'

Enhancements to Prometheus exporter #609

Enhancements to Prometheus exporter #609

Conversation

NajmudheenCT commented Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NajmudheenCT Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 16, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sushanthakumar Jun 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumarashit left a comment

Choose a reason for hiding this comment

sushanthakumar commented Jun 19, 2021

joseph-v left a comment

Choose a reason for hiding this comment

skdwriting left a comment

Choose a reason for hiding this comment

NajmudheenCT commented Jun 15, 2021 •

edited

Loading

NajmudheenCT Jun 16, 2021 •

edited

Loading

codecov bot commented Jun 16, 2021 •

edited

Loading

sushanthakumar Jun 18, 2021 •

edited

Loading