extract version utils and use semver for version comparison (#4844)

* Use semver for version comparison, extract versioning logic to vertion_utils

datadog_checks_base/datadog_checks/base/data/agent_requirements.in

@@ -53,6 +53,7 @@ requests_ntlm==1.1.0
 scandir==1.8
 securesystemslib[crypto,pynacl]==0.11.3
 selectors34==1.2.0; sys_platform == 'win32' and python_version < '3.4'
+semver==2.9.0
 serpent==1.27; sys_platform == 'win32'
 service_identity[idna]==18.1.0
 simplejson==3.6.5

postgres/datadog_checks/postgres/postgres.py

@@ -8,7 +8,6 @@
 
 import psycopg2
 from six import iteritems
-from six.moves import zip_longest
 
 from datadog_checks.base import AgentCheck, ConfigurationError, is_affirmative
 
@@ -41,6 +40,7 @@
     STATIO_METRICS,
     fmt,
 )
+from .version_utils import V8_3, V9, V9_1, V9_2, V9_4, V9_6, V10, get_version
 
 MAX_CUSTOM_RESULTS = 100
 TABLE_COUNT_LIMIT = 200
@@ -64,6 +64,7 @@ def __init__(self, name, init_config, instances):
         AgentCheck.__init__(self, name, init_config, instances)
         self._clean_state()
         self.db = None
+        self._version = None
         self.custom_metrics = None
 
         # Deprecate custom_metrics in favor of custom_queries
@@ -104,7 +105,7 @@ def _build_tags(self, custom_tags, host, port, dbname):
         return tags
 
     def _clean_state(self):
-        self.version = None
+        self._version = None
         self.instance_metrics = None
         self.bgw_metrics = None
         self.archiver_metrics = None
@@ -120,62 +121,12 @@ def _get_replication_role(self):
         # value fetched for role is of <type 'bool'>
         return "standby" if role else "master"
 
-    def _get_version(self):
-        if self.version is None:
-            cursor = self.db.cursor()
-            cursor.execute('SHOW SERVER_VERSION;')
-            version = cursor.fetchone()[0]
-            try:
-                version_parts = version.split(' ')[0].split('.')
-                version = [int(part) for part in version_parts]
-            except Exception:
-                # Postgres might be in development, with format \d+[beta|rc]\d+
-                match = re.match(r'(\d+)([a-zA-Z]+)(\d+)', version)
-                if match:
-                    version_parts = list(match.groups())
-
-                    # We found a valid development version
-                    if len(version_parts) == 3:
-                        # Replace development tag with a negative number to properly compare versions
-                        version_parts[1] = -1
-                        version = [int(part) for part in version_parts]
-
-            self.version = version
-
-        self.service_metadata('version', self.version)
-        return self.version
-
-    def _is_above(self, version_to_compare):
-        version = self._get_version()
-        if type(version) == list:
-            # iterate from major down to bugfix
-            for v, vc in zip_longest(version, version_to_compare, fillvalue=0):
-                if v == vc:
-                    continue
-
-                return v > vc
-
-            # return True if version is the same
-            return True
-        return False
-
-    def _is_8_3_or_above(self):
-        return self._is_above([8, 3, 0])
-
-    def _is_9_1_or_above(self):
-        return self._is_above([9, 1, 0])
-
-    def _is_9_2_or_above(self):
-        return self._is_above([9, 2, 0])
-
-    def _is_9_4_or_above(self):
-        return self._is_above([9, 4, 0])
-
-    def _is_9_6_or_above(self):
-        return self._is_above([9, 6, 0])
-
-    def _is_10_or_above(self):
-        return self._is_above([10, 0, 0])
+    @property
+    def version(self):
+        if self._version is None:
+            self._version = get_version(self.db)
+            self.service_metadata('version', [self._version.major, self._version.minor, self._version.patch])
+        return self._version
 
     def _get_instance_metrics(self, database_size_metrics, collect_default_db):
         """
@@ -193,7 +144,7 @@ def _get_instance_metrics(self, database_size_metrics, collect_default_db):
 
         if metrics is None:
             # select the right set of metrics to collect depending on postgres version
-            if self._is_9_2_or_above():
+            if self.version >= V9_2:
                 self.instance_metrics = dict(COMMON_METRICS, **NEWER_92_METRICS)
             else:
                 self.instance_metrics = dict(COMMON_METRICS)
@@ -242,11 +193,11 @@ def _get_bgw_metrics(self):
                 return None
 
             self.db_bgw_metrics.append(sub_key)
-
             self.bgw_metrics = dict(COMMON_BGW_METRICS)
-            if self._is_9_1_or_above():
+
+            if self.version >= V9_1:
                 self.bgw_metrics.update(NEWER_91_BGW_METRICS)
-            if self._is_9_2_or_above():
+            if self.version >= V9_2:
                 self.bgw_metrics.update(NEWER_92_BGW_METRICS)
 
             metrics = self.bgw_metrics
@@ -277,7 +228,7 @@ def _get_archiver_metrics(self):
         # the table, mirroring _get_bgw_metrics()
         metrics = self.archiver_metrics
 
-        if metrics is None and self._is_9_4_or_above():
+        if metrics is None and self.version >= V9_4:
             # Collect from only one instance. See _get_bgw_metrics() for details on why.
             sub_key = self.key[:2]
             if sub_key in self.db_archiver_metrics:
@@ -310,12 +261,12 @@ def _get_replication_metrics(self):
         Uses a dictionnary to save the result for each instance
         """
         metrics = self.replication_metrics
-        if self._is_10_or_above() and metrics is None:
+        if self.version >= V10 and metrics is None:
             self.replication_metrics = dict(REPLICATION_METRICS_10)
             metrics = self.replication_metrics
-        elif self._is_9_1_or_above() and metrics is None:
+        elif self.version >= V9_1 and metrics is None:
             self.replication_metrics = dict(REPLICATION_METRICS_9_1)
-            if self._is_9_2_or_above():
+            if self.version >= V9_2:
                 self.replication_metrics.update(REPLICATION_METRICS_9_2)
             metrics = self.replication_metrics
         return metrics
@@ -328,12 +279,12 @@ def _get_activity_metrics(self, user):
         metrics_data = self.activity_metrics
 
         if metrics_data is None:
-            query = ACTIVITY_QUERY_10 if self._is_10_or_above() else ACTIVITY_QUERY_LT_10
-            if self._is_9_6_or_above():
+            query = ACTIVITY_QUERY_10 if self.version >= V10 else ACTIVITY_QUERY_LT_10
+            if self.version >= V9_6:
                 metrics_query = ACTIVITY_METRICS_9_6
-            elif self._is_9_2_or_above():
+            elif self.version >= V9_2:
                 metrics_query = ACTIVITY_METRICS_9_2
-            elif self._is_8_3_or_above():
+            elif self.version >= V8_3:
                 metrics_query = ACTIVITY_METRICS_8_3
             else:
                 metrics_query = ACTIVITY_METRICS_LT_8_3
@@ -384,8 +335,7 @@ def _build_relations_config(self, yamlconfig):
     def _query_scope(self, cursor, scope, instance_tags, is_custom_metrics, relations_config):
         if scope is None:
             return None
-
-        if scope == REPLICATION_METRICS or not self._is_above([9, 0, 0]):
+        if scope == REPLICATION_METRICS or not self.version >= V9:
             log_func = self.log.debug
         else:
             log_func = self.log.warning
@@ -413,7 +363,7 @@ def _query_scope(self, cursor, scope, instance_tags, is_custom_metrics, relation
             log_func(e)
             log_func(
                 "It seems the PG version has been incorrectly identified as %s. "
-                "A reattempt to identify the right version will happen on next agent run." % self.version
+                "A reattempt to identify the right version will happen on next agent run." % self._version
             )
             self._clean_state()
             self.db.rollback()
@@ -772,8 +722,7 @@ def check(self, instance):
             self._connect(host, port, user, password, dbname, ssl, tags)
             if tag_replication_role:
                 tags.extend(["replication_role:{}".format(self._get_replication_role())])
-            version = self._get_version()
-            self.log.debug("Running check against version %s" % version)
+            self.log.debug("Running check against version %s", self.version)
             self._collect_stats(
                 user,
                 tags,

postgres/datadog_checks/postgres/version_utils.py

@@ -0,0 +1,38 @@
+# (C) Datadog, Inc. 2019
+# All rights reserved
+# Licensed under Simplified BSD License (see LICENSE)
+import re
+
+import semver
+
+V8_3 = semver.parse("8.3.0")
+V9 = semver.parse("9.0.0")
+V9_1 = semver.parse("9.1.0")
+V9_2 = semver.parse("9.2.0")
+V9_4 = semver.parse("9.4.0")
+V9_6 = semver.parse("9.6.0")
+V10 = semver.parse("10.0.0")
+
+
+def get_version(db):
+    cursor = db.cursor()
+    cursor.execute('SHOW SERVER_VERSION;')
+    raw_version = cursor.fetchone()[0]
+    try:
+        version_parts = raw_version.split(' ')[0].split('.')
+        version = [int(part) for part in version_parts]
+        while len(version) < 3:
+            version.append(0)
+        return semver.VersionInfo(*version)
+    except Exception:
+        # Postgres might be in development, with format \d+[beta|rc]\d+
+        match = re.match(r'(\d+)([a-zA-Z]+)(\d+)', raw_version)
+        if match:
+            version = list(match.groups())
+            # We found a valid development version
+            if len(version) == 3:
+                # Replace development tag with a negative number to properly compare versions
+                version[1] = -1
+                version = [int(part) for part in version]
+            return semver.VersionInfo(*version)
+    raise Exception("Cannot determine which version is {}".format(raw_version))

postgres/requirements.in

@@ -1 +1,2 @@
 psycopg2-binary==2.8.4
+semver==2.9.0

postgres/tests/conftest.py

@@ -3,9 +3,9 @@
 # Licensed under Simplified BSD License (see LICENSE)
 import os
 
-import mock
 import psycopg2
 import pytest
+from semver import VersionInfo
 
 from datadog_checks.dev import WaitFor, docker_run
 from datadog_checks.postgres import PostgreSql
@@ -31,15 +31,14 @@ def dd_environment(e2e_instance):
 @pytest.fixture
 def check():
     c = PostgreSql('postgres', {}, [{'dbname': 'dbname', 'host': 'localhost', 'port': '5432'}])
-    c._is_9_2_or_above = mock.MagicMock()
+    c._version = VersionInfo(9, 2, 0)
     return c
 
 
 @pytest.fixture
 def integration_check():
     def _check(instance):
         c = PostgreSql('postgres', {}, [instance])
-        c._is_9_2_or_above = mock.MagicMock()
         return c
 
     return _check

postgres/tests/test_integration.py

@@ -6,6 +6,7 @@
 import mock
 import psycopg2
 import pytest
+from semver import VersionInfo
 
 from datadog_checks.postgres import PostgreSql
 
@@ -126,13 +127,13 @@ def test_activity_metrics(aggregator, integration_check, pg_instance):
 def test_wrong_version(aggregator, integration_check, pg_instance):
     check = integration_check(pg_instance)
     # Enforce to cache wrong version
-    check.version = [9, 6, 0]
+    check._version = VersionInfo(*[9, 6, 0])
 
     check.check(pg_instance)
     assert_state_clean(check)
 
     check.check(pg_instance)
-    assert check.version[0] == int(POSTGRES_VERSION)
+    assert check._version.major == int(POSTGRES_VERSION)
     assert_state_set(check)
 
 
@@ -158,7 +159,7 @@ def throw_exception_first_time(*args, **kwargs):
 
 
 def assert_state_clean(check):
-    assert check.version is None
+    assert check._version is None
     assert check.instance_metrics is None
     assert check.bgw_metrics is None
     assert check.archiver_metrics is None
@@ -169,7 +170,7 @@ def assert_state_clean(check):
 
 
 def assert_state_set(check,):
-    assert check.version
+    assert check._version
     assert check.instance_metrics
     assert check.bgw_metrics
     if POSTGRES_VERSION != '9.3':