From a3e4b86af1d8668198a7410e66b15bf047195299 Mon Sep 17 00:00:00 2001 From: Johan Fischer Date: Wed, 3 Apr 2019 06:15:16 +0000 Subject: [PATCH 1/3] Fixed schema filtering on postgres relations (#3440) Fixed the loading of the configuration file (avoid KeyError and check yaml structure). Fix filtering loop on query_scope to continue on other rows. Simplify parameter and removed 'relations' as 'relations_config' contains it all. --- postgres/datadog_checks/postgres/postgres.py | 74 ++++++++++++-------- 1 file changed, 46 insertions(+), 28 deletions(-) diff --git a/postgres/datadog_checks/postgres/postgres.py b/postgres/datadog_checks/postgres/postgres.py index 7b95183756c7c..99b7e0b494537 100644 --- a/postgres/datadog_checks/postgres/postgres.py +++ b/postgres/datadog_checks/postgres/postgres.py @@ -671,21 +671,28 @@ def _build_relations_config(self, yamlconfig): """ config = {} for element in yamlconfig: - try: - if isinstance(element, str): - config[element] = {'relation_name': element, 'schemas': []} - elif isinstance(element, dict): - name = element['relation_name'] - config[name] = {} - config[name]['schemas'] = element['schemas'] - config[name]['relation_name'] = name - else: - self.log.warning('Unhandled relations config type: {}'.format(element)) - except KeyError: - self.log.warning('Failed to parse config element={}, check syntax'.format(element)) + if isinstance(element, str): + config[element] = { + 'relation_name': element, + 'schemas': [] + } + elif isinstance(element, dict): + if 'relation_name' not in element or 'schemas' not in element: + self.log.warning("Unknown element format for relation element %s", element) + continue + if not isinstance(element['schemas'], list): + self.log.warning("Expected a list of schemas for %s", element) + continue + name = element['relation_name'] + config[name] = { + 'relation_name': name, + 'schemas': element['schemas'], + } + else: + self.log.warning('Unhandled relations config type: {}'.format(element)) return config - def _query_scope(self, cursor, scope, key, db, instance_tags, relations, is_custom_metrics, programming_error, + def _query_scope(self, cursor, scope, key, db, instance_tags, is_custom_metrics, programming_error, relations_config): if scope is None: return None @@ -701,7 +708,7 @@ def _query_scope(self, cursor, scope, key, db, instance_tags, relations, is_cust try: # if this is a relation-specific query, we need to list all relations last - if scope['relation'] and len(relations) > 0: + if scope['relation'] and len(relations_config) > 0: relnames = ', '.join("'{0}'".format(w) for w in relations_config) query = scope['query'] % (", ".join(cols), "%s") # Keep the last %s intact self.log.debug("Running query: %s with relations: %s" % (query, relnames)) @@ -733,20 +740,30 @@ def _query_scope(self, cursor, scope, key, db, instance_tags, relations, is_cust # A row should look like this # (descriptor, descriptor, ..., value, value, value, value, ...) # with descriptor a PG relation or index name, which we use to create the tags + valid_results_size = 0 for row in results: # Check that all columns will be processed assert len(row) == len(cols) + len(desc) # build a map of descriptors and their values desc_map = dict(zip([x[1] for x in desc], row[0:len(desc)])) - if 'schema' in desc_map and relations: - try: - relname = desc_map['table'] - config_schemas = relations_config[relname]['schemas'] - if config_schemas and desc_map['schema'] not in config_schemas: - return len(results) - except KeyError: - pass + + # if relations *and* schemas are set, filter out table not + # matching the schema in the configuration + if scope['relation'] and len(relations_config) > 0 and 'schema' in desc_map and 'table' in desc_map: + row_table = desc_map['table'] + row_schema = desc_map['schema'] + + config_table_obj = relations_config.get(row_table) + if not config_table_obj: + self.log.info("weird, got row %s.%s, but not relation", row_schema, row_table) + else: + config_schemas = config_table_obj.get('schemas') + if not config_schemas: + self.log.debug("all schemas are allowed for table %s.%s", row_schema, row_table) + elif row_schema not in config_schemas: + self.log.debug("Skipping non match schema %s for table %s", desc_map['schema'], row_table) + continue # Build tags # descriptors are: (pg_name, dd_tag_name): value @@ -769,8 +786,9 @@ def _query_scope(self, cursor, scope, key, db, instance_tags, relations, is_cust # tags are for v in zip([scope['metrics'][c] for c in cols], row[len(desc):]): v[0][1](self, v[0][0], v[1], tags=tags) + valid_results_size += 1 - return len(results) + return valid_results_size def _collect_stats(self, key, db, instance_tags, relations, custom_metrics, collect_function_metrics, collect_count_metrics, collect_activity_metrics, collect_database_size_metrics, @@ -814,24 +832,24 @@ def _collect_stats(self, key, db, instance_tags, relations, custom_metrics, coll try: cursor = db.cursor() - results_len = self._query_scope(cursor, db_instance_metrics, key, db, instance_tags, relations, + results_len = self._query_scope(cursor, db_instance_metrics, key, db, instance_tags, False, programming_error, relations_config) if results_len is not None: self.gauge("postgresql.db.count", results_len, tags=[t for t in instance_tags if not t.startswith("db:")]) - self._query_scope(cursor, bgw_instance_metrics, key, db, instance_tags, relations, + self._query_scope(cursor, bgw_instance_metrics, key, db, instance_tags, False, programming_error, relations_config) - self._query_scope(cursor, archiver_instance_metrics, key, db, instance_tags, relations, + self._query_scope(cursor, archiver_instance_metrics, key, db, instance_tags, False, programming_error, relations_config) if collect_activity_metrics: activity_metrics = self._get_activity_metrics(key, db) - self._query_scope(cursor, activity_metrics, key, db, instance_tags, relations, + self._query_scope(cursor, activity_metrics, key, db, instance_tags, False, programming_error, relations_config) for scope in list(metric_scope) + custom_metrics: - self._query_scope(cursor, scope, key, db, instance_tags, relations, + self._query_scope(cursor, scope, key, db, instance_tags, scope in custom_metrics, programming_error, relations_config) cursor.close() From b3980f1ecd219130925c8db4a41f8e9dcaaf0006 Mon Sep 17 00:00:00 2001 From: Ofek Lev Date: Mon, 22 Apr 2019 17:50:59 -0400 Subject: [PATCH 2/3] fix style --- postgres/datadog_checks/postgres/postgres.py | 55 +++----------------- 1 file changed, 7 insertions(+), 48 deletions(-) diff --git a/postgres/datadog_checks/postgres/postgres.py b/postgres/datadog_checks/postgres/postgres.py index 0cf5011d8b812..d0bee32373382 100644 --- a/postgres/datadog_checks/postgres/postgres.py +++ b/postgres/datadog_checks/postgres/postgres.py @@ -642,10 +642,7 @@ def _build_relations_config(self, yamlconfig): config = {} for element in yamlconfig: if isinstance(element, str): - config[element] = { - 'relation_name': element, - 'schemas': [] - } + config[element] = {'relation_name': element, 'schemas': []} elif isinstance(element, dict): if 'relation_name' not in element or 'schemas' not in element: self.log.warning("Unknown element format for relation element %s", element) @@ -654,10 +651,7 @@ def _build_relations_config(self, yamlconfig): self.log.warning("Expected a list of schemas for %s", element) continue name = element['relation_name'] - config[name] = { - 'relation_name': name, - 'schemas': element['schemas'], - } + config[name] = {'relation_name': name, 'schemas': element['schemas']} else: self.log.warning('Unhandled relations config type: {}'.format(element)) return config @@ -809,14 +803,7 @@ def _collect_stats( try: cursor = db.cursor() results_len = self._query_scope( - cursor, - db_instance_metrics, - key, - db, - instance_tags, - False, - programming_error, - relations_config, + cursor, db_instance_metrics, key, db, instance_tags, False, programming_error, relations_config ) if results_len is not None: self.gauge( @@ -824,49 +811,21 @@ def _collect_stats( ) self._query_scope( - cursor, - bgw_instance_metrics, - key, - db, - instance_tags, - False, - programming_error, - relations_config, + cursor, bgw_instance_metrics, key, db, instance_tags, False, programming_error, relations_config ) self._query_scope( - cursor, - archiver_instance_metrics, - key, - db, - instance_tags, - False, - programming_error, - relations_config, + cursor, archiver_instance_metrics, key, db, instance_tags, False, programming_error, relations_config ) if collect_activity_metrics: activity_metrics = self._get_activity_metrics(key, db) self._query_scope( - cursor, - activity_metrics, - key, - db, - instance_tags, - False, - programming_error, - relations_config, + cursor, activity_metrics, key, db, instance_tags, False, programming_error, relations_config ) for scope in list(metric_scope) + custom_metrics: self._query_scope( - cursor, - scope, - key, - db, - instance_tags, - scope in custom_metrics, - programming_error, - relations_config, + cursor, scope, key, db, instance_tags, scope in custom_metrics, programming_error, relations_config ) cursor.close() From 0419ffe5cf6769899a199f6ac903b37a314e7af3 Mon Sep 17 00:00:00 2001 From: Ofek Lev Date: Mon, 22 Apr 2019 18:23:44 -0400 Subject: [PATCH 3/3] log --- postgres/datadog_checks/postgres/postgres.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/postgres/datadog_checks/postgres/postgres.py b/postgres/datadog_checks/postgres/postgres.py index d0bee32373382..26c22428f18fc 100644 --- a/postgres/datadog_checks/postgres/postgres.py +++ b/postgres/datadog_checks/postgres/postgres.py @@ -722,13 +722,13 @@ def _query_scope( config_table_obj = relations_config.get(row_table) if not config_table_obj: - self.log.info("weird, got row %s.%s, but not relation", row_schema, row_table) + self.log.info("Got row %s.%s, but not relation", row_schema, row_table) else: config_schemas = config_table_obj.get('schemas') if not config_schemas: - self.log.debug("all schemas are allowed for table %s.%s", row_schema, row_table) + self.log.debug("All schemas are allowed for table %s.%s", row_schema, row_table) elif row_schema not in config_schemas: - self.log.debug("Skipping non match schema %s for table %s", desc_map['schema'], row_table) + self.log.debug("Skipping non matched schema %s for table %s", desc_map['schema'], row_table) continue # Build tags