From a9458a9b00951f938ffe8acd9c86696d5c31c67e Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Thu, 22 Feb 2024 11:53:49 -0500 Subject: [PATCH] fixes salt-extensions/saltext-prometheus#26 salt_aborted metric lacks a state label --- changelog/26.fixed.md | 1 + .../returners/prometheus_textfile.py | 25 +- .../test_prometheus_textfile_return.py | 213 ++++++++++++++---- 3 files changed, 193 insertions(+), 46 deletions(-) create mode 100644 changelog/26.fixed.md diff --git a/changelog/26.fixed.md b/changelog/26.fixed.md new file mode 100644 index 0000000..952edd1 --- /dev/null +++ b/changelog/26.fixed.md @@ -0,0 +1 @@ +Fix salt_aborted metric lacking a state label diff --git a/src/saltext/prometheus/returners/prometheus_textfile.py b/src/saltext/prometheus/returners/prometheus_textfile.py index 917f57f..90edf6e 100644 --- a/src/saltext/prometheus/returners/prometheus_textfile.py +++ b/src/saltext/prometheus/returners/prometheus_textfile.py @@ -373,16 +373,31 @@ def returner(ret): gauge_show_failed_states.labels(*label_values).set(1) if opts["abort_state_ids"]: + labels = [] + label_values = [] + if opts["add_state_name"]: + labels.append("state") + label_values.append(prom_state) if not isinstance(opts["abort_state_ids"], list): opts["abort_state_ids"] = [item.strip() for item in opts["abort_state_ids"].split(",")] - gauge_salt_aborted = Gauge( - "salt_aborted", "Flag to show that a specific abort state failed", registry=registry - ) - gauge_salt_aborted.set(0) + aborted_value = 0 for state_id, state_return in ret["return"].items(): if not state_return["result"] and state_return.get("__id__") in opts["abort_state_ids"]: - gauge_salt_aborted.set(1) + aborted_value = 1 + labels.append("state_id") + label_values.append(state_id.split("_|-")[1]) + + gauge_salt_aborted = Gauge( + "salt_aborted", + "Flag to show that a specific abort state failed", + labels, + registry=registry, + ) + if label_values: + gauge_salt_aborted.labels(*label_values).set(aborted_value) + else: + gauge_salt_aborted.set(aborted_value) if opts["add_state_name"]: old_name, ext = os.path.splitext(opts["filename"]) diff --git a/tests/unit/returners/test_prometheus_textfile_return.py b/tests/unit/returners/test_prometheus_textfile_return.py index f2bfe03..522807c 100644 --- a/tests/unit/returners/test_prometheus_textfile_return.py +++ b/tests/unit/returners/test_prometheus_textfile_return.py @@ -12,17 +12,17 @@ from tests.support.mock import patch -@pytest.fixture +@pytest.fixture() def root_dir(tmp_path): return str(tmp_path / "root_dir") -@pytest.fixture +@pytest.fixture() def cache_dir(root_dir): return os.path.join(root_dir, "cachedir") -@pytest.fixture +@pytest.fixture(scope="function") def job_ret(): ret = { "jid": "20211109174620871797", @@ -70,17 +70,14 @@ def job_ret(): return ret -@pytest.fixture -def patch_dunders(cache_dir, minion): +@pytest.fixture(scope="function") +def opts(cache_dir, minion): opts = minion.config.copy() opts["cachedir"] = cache_dir - with patch( - "saltext.prometheus.returners.prometheus_textfile.__opts__", opts, create=True - ), patch("saltext.prometheus.returners.prometheus_textfile.__salt__", {}, create=True): - yield + return opts -def test_basic_prometheus_output_with_default_options(patch_dunders, job_ret, cache_dir, minion): +def test_basic_prometheus_output_with_default_options(job_ret, cache_dir, opts): expected = "\n".join( sorted( [ @@ -127,7 +124,10 @@ def test_basic_prometheus_output_with_default_options(patch_dunders, job_ret, ca ) ) - prometheus_textfile.returner(job_ret) + with patch( + "saltext.prometheus.returners.prometheus_textfile.__opts__", opts, create=True + ), patch("saltext.prometheus.returners.prometheus_textfile.__salt__", {}, create=True): + prometheus_textfile.returner(job_ret) with salt.utils.files.fopen( os.path.join(cache_dir, "prometheus_textfile", "salt.prom") @@ -154,18 +154,15 @@ def test_basic_prometheus_output_with_default_options(patch_dunders, job_ret, ca ], ) def test_when_add_state_name_is_set_then_correct_output_should_be_in_correct_file( - patch_dunders, state_name, filename, expected_filename, - minion, + opts, cache_dir, job_ret, ): job_ret["fun_args"][0] = state_name - prometheus_textfile.__opts__.update( - {"add_state_name": True, "filename": os.path.join(cache_dir, filename)} - ) + opts.update({"add_state_name": True, "filename": os.path.join(cache_dir, filename)}) expected = "\n".join( sorted( @@ -214,7 +211,10 @@ def test_when_add_state_name_is_set_then_correct_output_should_be_in_correct_fil ] ) ) - prometheus_textfile.returner(job_ret) + with patch( + "saltext.prometheus.returners.prometheus_textfile.__opts__", opts, create=True + ), patch("saltext.prometheus.returners.prometheus_textfile.__salt__", {}, create=True): + prometheus_textfile.returner(job_ret) with salt.utils.files.fopen(os.path.join(cache_dir, expected_filename)) as prom_file: # use line[:-1] to strip off the newline, but only one. It may be extra @@ -231,10 +231,10 @@ def test_when_add_state_name_is_set_then_correct_output_should_be_in_correct_fil def test_prometheus_output_with_show_failed_state_option_and_abort_state_ids( - patch_dunders, job_ret, cache_dir, minion + job_ret, cache_dir, opts ): job_ret["return"]["cmd_|-echo includeme_|-echo includeme_|-run"]["result"] = False - prometheus_textfile.__opts__.update({"show_failed_states": True}) + opts.update({"show_failed_states": True}) promfile_lines = [ "# HELP salt_procs Number of salt minion processes running", "# TYPE salt_procs gauge", @@ -283,7 +283,10 @@ def test_prometheus_output_with_show_failed_state_option_and_abort_state_ids( # Test one failed state expected = "\n".join(sorted(promfile_lines)) - prometheus_textfile.returner(job_ret) + with patch( + "saltext.prometheus.returners.prometheus_textfile.__opts__", opts, create=True + ), patch("saltext.prometheus.returners.prometheus_textfile.__salt__", {}, create=True): + prometheus_textfile.returner(job_ret) with salt.utils.files.fopen( os.path.join(cache_dir, "prometheus_textfile", "salt.prom") @@ -310,7 +313,10 @@ def test_prometheus_output_with_show_failed_state_option_and_abort_state_ids( ) expected = "\n".join(sorted(promfile_lines)) - prometheus_textfile.returner(job_ret) + with patch( + "saltext.prometheus.returners.prometheus_textfile.__opts__", opts, create=True + ), patch("saltext.prometheus.returners.prometheus_textfile.__salt__", {}, create=True): + prometheus_textfile.returner(job_ret) with salt.utils.files.fopen( os.path.join(cache_dir, "prometheus_textfile", "salt.prom") @@ -327,16 +333,19 @@ def test_prometheus_output_with_show_failed_state_option_and_abort_state_ids( assert salt_prom == expected # Test abort state ID - prometheus_textfile.__opts__.update({"abort_state_ids": ["echo includeme"]}) + opts.update({"abort_state_ids": ["echo includeme"]}) promfile_lines.extend( [ "# HELP salt_aborted Flag to show that a specific abort state failed", "# TYPE salt_aborted gauge", - "salt_aborted 1.0", + 'salt_aborted{state_id="echo includeme"} 1.0', ] ) expected = "\n".join(sorted(promfile_lines)) - prometheus_textfile.returner(job_ret) + with patch( + "saltext.prometheus.returners.prometheus_textfile.__opts__", opts, create=True + ), patch("saltext.prometheus.returners.prometheus_textfile.__salt__", {}, create=True): + prometheus_textfile.returner(job_ret) with salt.utils.files.fopen( os.path.join(cache_dir, "prometheus_textfile", "salt.prom") @@ -353,8 +362,8 @@ def test_prometheus_output_with_show_failed_state_option_and_abort_state_ids( assert salt_prom == expected -def test_fail_comments_lengths(patch_dunders, job_ret, cache_dir, minion): - prometheus_textfile.__opts__.update({"show_failed_states": True}) +def test_fail_comments_lengths(job_ret, cache_dir, opts): + opts.update({"show_failed_states": True}) promfile_lines = [ "# HELP salt_procs Number of salt minion processes running", "# TYPE salt_procs gauge", @@ -403,14 +412,17 @@ def test_fail_comments_lengths(patch_dunders, job_ret, cache_dir, minion): # Test two failed states with no comment length limit - prometheus_textfile.__opts__.update({"fail_comment_length": None}) + opts.update({"fail_comment_length": None}) expected = "\n".join(sorted(promfile_lines)) job_ret["return"]["cmd_|-echo includeme_|-echo includeme_|-run"]["result"] = False job_ret["return"]["cmd_|-echo applyme_|-echo applyme_|-run"]["result"] = False - prometheus_textfile.returner(job_ret) + with patch( + "saltext.prometheus.returners.prometheus_textfile.__opts__", opts, create=True + ), patch("saltext.prometheus.returners.prometheus_textfile.__salt__", {}, create=True): + prometheus_textfile.returner(job_ret) with salt.utils.files.fopen( os.path.join(cache_dir, "prometheus_textfile", "salt.prom") @@ -474,14 +486,17 @@ def test_fail_comments_lengths(patch_dunders, job_ret, cache_dir, minion): # Test two failed states with comment length limit of 15 - prometheus_textfile.__opts__.update({"fail_comment_length": 15}) + opts.update({"fail_comment_length": 15}) expected = "\n".join(sorted(promfile_lines)) job_ret["return"]["cmd_|-echo includeme_|-echo includeme_|-run"]["result"] = False job_ret["return"]["cmd_|-echo applyme_|-echo applyme_|-run"]["result"] = False - prometheus_textfile.returner(job_ret) + with patch( + "saltext.prometheus.returners.prometheus_textfile.__opts__", opts, create=True + ), patch("saltext.prometheus.returners.prometheus_textfile.__salt__", {}, create=True): + prometheus_textfile.returner(job_ret) with salt.utils.files.fopen( os.path.join(cache_dir, "prometheus_textfile", "salt.prom") @@ -499,14 +514,20 @@ def test_fail_comments_lengths(patch_dunders, job_ret, cache_dir, minion): assert salt_prom == expected -def test_prometheus_output_with_raw_version(patch_dunders, job_ret, cache_dir, minion): +def test_prometheus_output_with_raw_version(job_ret, cache_dir, opts): expected_version = "3004+12.g557e6cc0fc" short_version = expected_version.split("+", maxsplit=1)[0] float_version = str(float(short_version)) - prometheus_textfile.__grains__.update({"saltversion": expected_version}) # raw_version == False - prometheus_textfile.returner(job_ret) + with patch( + "saltext.prometheus.returners.prometheus_textfile.__opts__", opts, create=True + ), patch("saltext.prometheus.returners.prometheus_textfile.__salt__", {}, create=True), patch( + "saltext.prometheus.returners.prometheus_textfile.__grains__", + {"saltversion": expected_version}, + create=True, + ): + prometheus_textfile.returner(job_ret) with salt.utils.files.fopen( os.path.join(cache_dir, "prometheus_textfile", "salt.prom") @@ -524,8 +545,15 @@ def test_prometheus_output_with_raw_version(patch_dunders, job_ret, cache_dir, m assert salt_version_tagged == short_version # raw_version == True - prometheus_textfile.__opts__.update({"raw_version": True}) - prometheus_textfile.returner(job_ret) + opts.update({"raw_version": True}) + with patch( + "saltext.prometheus.returners.prometheus_textfile.__opts__", opts, create=True + ), patch("saltext.prometheus.returners.prometheus_textfile.__salt__", {}, create=True), patch( + "saltext.prometheus.returners.prometheus_textfile.__grains__", + {"saltversion": expected_version}, + create=True, + ): + prometheus_textfile.returner(job_ret) with salt.utils.files.fopen( os.path.join(cache_dir, "prometheus_textfile", "salt.prom") @@ -543,7 +571,7 @@ def test_prometheus_output_with_raw_version(patch_dunders, job_ret, cache_dir, m assert salt_version_tagged == expected_version -def test_requisite_handling(patch_dunders, cache_dir, minion): +def test_requisite_handling(cache_dir, opts): job_ret = { "fun": "state.apply", "fun_args": ["prom_ret"], @@ -576,18 +604,121 @@ def test_requisite_handling(patch_dunders, cache_dir, minion): "success": True, } - prometheus_textfile.__opts__.update({"abort_state_ids": ["echo includeme"]}) - prometheus_textfile.returner(job_ret) + opts.update({"abort_state_ids": ["echo includeme"]}) + with patch( + "saltext.prometheus.returners.prometheus_textfile.__opts__", opts, create=True + ), patch("saltext.prometheus.returners.prometheus_textfile.__salt__", {}, create=True): + prometheus_textfile.returner(job_ret) assert Path(os.path.join(cache_dir, "prometheus_textfile", "salt.prom")).exists() @pytest.mark.skip_on_windows(reason="mode setting not available on Windows") -def test_mode_passed_to_set_mode(patch_dunders, cache_dir, job_ret, minion): +def test_mode_passed_to_set_mode(cache_dir, job_ret, opts): mock_set_mode = MagicMock(return_value=True) - prometheus_textfile.__opts__.update({"mode": "0644"}) - with patch("salt.modules.file.set_mode", mock_set_mode): + opts.update({"mode": "0644"}) + with patch( + "saltext.prometheus.returners.prometheus_textfile.__opts__", opts, create=True + ), patch("saltext.prometheus.returners.prometheus_textfile.__salt__", {}, create=True), patch( + "salt.modules.file.set_mode", mock_set_mode + ): prometheus_textfile.returner(job_ret) mock_set_mode.assert_called_with( os.path.join(cache_dir, "prometheus_textfile", "salt.prom"), "0644" ) + + +@pytest.mark.parametrize( + "state_name,filename,expected_filename", + [ + ("aaa", "one", "one-aaa"), + ("bbb", "one.two", "one-bbb.two"), + ("ccc", "one.two.three", "one.two-ccc.three"), + ("ddd", "one.two.three.four", "one.two.three-ddd.four"), + ], +) +def test_add_state_name_adds_salt_aborted_label( + state_name, + filename, + expected_filename, + opts, + cache_dir, + job_ret, +): + job_ret["fun_args"][0] = state_name + job_ret["return"]["cmd_|-echo includeme_|-echo includeme_|-run"]["result"] = False + opts.update( + { + "add_state_name": True, + "filename": os.path.join(cache_dir, filename), + "abort_state_ids": ["echo includeme"], + } + ) + + expected = "\n".join( + sorted( + [ + "# HELP salt_procs Number of salt minion processes running", + "# TYPE salt_procs gauge", + f'salt_procs{{state="{state_name}"}} 0.0', + "# HELP salt_states_succeeded Number of successful states in the run", + "# TYPE salt_states_succeeded gauge", + f'salt_states_succeeded{{state="{state_name}"}} 1.0', + "# HELP salt_states_failed Number of failed states in the run", + "# TYPE salt_states_failed gauge", + f'salt_states_failed{{state="{state_name}"}} 1.0', + "# HELP salt_states_changed Number of changed states in the run", + "# TYPE salt_states_changed gauge", + f'salt_states_changed{{state="{state_name}"}} 2.0', + "# HELP salt_states_total Total states in the run", + "# TYPE salt_states_total gauge", + f'salt_states_total{{state="{state_name}"}} 2.0', + "# HELP salt_states_success_pct Percent of successful states in the run", + "# TYPE salt_states_success_pct gauge", + f'salt_states_success_pct{{state="{state_name}"}} 50.0', + "# HELP salt_states_failure_pct Percent of failed states in the run", + "# TYPE salt_states_failure_pct gauge", + f'salt_states_failure_pct{{state="{state_name}"}} 50.0', + "# HELP salt_states_changed_pct Percent of changed states in the run", + "# TYPE salt_states_changed_pct gauge", + f'salt_states_changed_pct{{state="{state_name}"}} 100.0', + "# HELP salt_elapsed_time Time spent for all operations during the state run", + "# TYPE salt_elapsed_time gauge", + f'salt_elapsed_time{{state="{state_name}"}} 13.695', + "# HELP salt_last_started Estimated time the state run started", + "# TYPE salt_last_started gauge", + "# HELP salt_last_completed Time of last state run completion", + "# TYPE salt_last_completed gauge", + "# HELP salt_version Version of installed Salt package", + "# TYPE salt_version gauge", + 'salt_version{{state="{}"}} {}'.format( # pylint: disable=consider-using-f-string + state_name, salt.version.__version__.split("rc", maxsplit=1)[0] + ), + "# HELP salt_version_tagged Version of installed Salt package as a tag", + "# TYPE salt_version_tagged gauge", + 'salt_version_tagged{{salt_version="{}",state="{}"}} 1.0'.format( # pylint: disable=consider-using-f-string + salt.version.__version__, state_name + ), + "# HELP salt_aborted Flag to show that a specific abort state failed", + "# TYPE salt_aborted gauge", + f'salt_aborted{{state="{state_name}",state_id="echo includeme"}} 1.0', + ] + ) + ) + with patch( + "saltext.prometheus.returners.prometheus_textfile.__opts__", opts, create=True + ), patch("saltext.prometheus.returners.prometheus_textfile.__salt__", {}, create=True): + prometheus_textfile.returner(job_ret) + + with salt.utils.files.fopen(os.path.join(cache_dir, expected_filename)) as prom_file: + # use line[:-1] to strip off the newline, but only one. It may be extra + # paranoid due to how Python file iteration works, but... + salt_prom = "\n".join( + sorted( + line[:-1] + for line in prom_file + if not line.startswith("salt_last_started") + and not line.startswith("salt_last_completed") + ) + ) + assert salt_prom == expected