Add first test for dedup by Ladas · Pull Request #166 · ansible/metrics-utility

Ladas · 2025-07-11T13:48:20Z

Issue: AAP-49237 & AAP-48948 & AAP-49231

Add a test demonstrating the dedup functionality

himdel

See review comments, but overall LGTM :)

sonarqubecloud · 2025-07-21T18:43:04Z

Please retry analysis of this Pull-Request directly on SonarQube Cloud

AlexSCorey

This code feels very dense to me. I've asked some questions mostly for my own clarity, though adding some details about how to test this would be helpful.

AlexSCorey · 2025-07-22T17:51:22Z


                billing_data['serial'] = billing_data.apply(compute_serial, axis=1)

+                experimental_dedup = self.extra_params.get('deduplicator') == 'ccsp-experimental'


I'm a bit weary of the experimental phrasing in this pr. If we are adding this to our code base it doesn't feel experimental any more, though I may be unaware of what part is experimental. Is the functionality experimental?

yeah, I think we should rename to something else in another PR, something like "ccsp-advanced" or mabe dropping the ccsp entirely...

AlexSCorey · 2025-07-22T18:33:14Z

+            'host_names_before_dedup': 'combine_set',
        }
+
+    def dedup(self, dataframe, hostname_mapping=None, scope_dataframe=None):


Looks like this function doesn't really dedupe anything. Should we call it something like facts_enrichment

this is overridden dedup method that then calls the parent with

return super().dedup(dataframe, hostname_mapping)

but, it might be more memory effective to enhance the jobhost summary dataframe with facts later (after the number of rows is reduced)

AlexSCorey · 2025-07-22T19:29:03Z

+        """Check if experimental deduplication is enabled."""
+        return self.extra_params.get('deduplicator') == 'ccsp-experimental'
+
+    def calculate_dedup_count(self, series):


Is this the number of hosts that were deduplicated?

this is number of host before the new dedup, there are both count and list of hostnames columns

so it shows how many hosts were grouped together with the new dedup

or better, if you see this new dedup count column being bigger than 1, that means this new deduplication kicked in and multiple hosts were deduped

Current approach is hacky, but it doesn;t require any code change. We'll need to unify the CI run and the local run, or create another docker compose.

… exception

Will help to debug deduplication issues

…/dataframe_jobhost_summary_usage.py Co-authored-by: Martin Hradil <mhradil@redhat.com>

Co-authored-by: Martin Hradil <mhradil@redhat.com>

if the fact key was missing, it was exploding

himdel · 2025-07-25T13:02:01Z

+    3.3. secure-host-01.company.com and secure-host-01-readonly.internal (privileged vs unprivileged):
+         - Same host accessed with different credentials
+         - Admin job has product_serial, user job doesn't
+         - Same machine_id in both cases 4f7a8b9c2d3e5f6a7b8c9d0e1f2a3b4c
+         - Result: Correctly merged based on machine_id
+         - Dedup: Working correctly - same machine_id causes deduplication


this is wrong - if the user job doesn't have product serial, and uses different hostnames, it won't get merged.
We don't merge on machine_id alone when product_serial is missing.

ah, yes, it's just a description error (they are correctly in false negatives category)

himdel · 2025-07-25T13:27:15Z

+    4.2. nat-host-01.external and nat-host-02.external:
+         - Different hosts behind same NAT gateway
+         - NAT gateway's machine_id and serial exposed to both
+         - Same public IP address (203.0.113.10) with different ports (2201 and 2202)
+         - Result: Still merged (port not used in deduplication logic)
+         - Dedup: Serial based on product_serial/machine_id only - port ignored
+         - Note: This demonstrates a limitation where NAT gateway hosts are incorrectly merged


suspicious - I'm seeing 2 different hostnames

why would the gateway's machine_id and serial propagate to both hosts?

(or is it because those are host_name, but ansible_host is 203.0.113.10 for both?)

yes, will fix the description

btw. I've added all original data into canonical facts, so these cases are visible:

'Canonical Facts': '{"ansible_host": ["203.0.113.10"], "ansible_machine_id": ' '["639d3a53a94028d35a3f5f244793dad2"], "ansible_port": [2201, 2202], ' '"ansible_product_serial": ["CN7792194B0NAT"], "host_name": ' '["nat-host-01.external", "nat-host-02.external"]}',

shows there was one ansible_host and 2 host_names

sonarqubecloud · 2025-07-25T13:42:50Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

* Add integration test testing new deduplication * Fix dedup in invetory_scope * Fix current test failing on new column thatshould show only with dedup * Refactoring of handling host names before dedup * Refactor host_names before dedup * Organize helper functions better * Add instructions on how to run tests in CI mode Current approach is hacky, but it doesn;t require any code change. We'll need to unify the CI run and the local run, or create another docker compose. * Test properly all sheets as dict comparissions * Simplify host_names_before_decup * Fix host before dedup and mock of report date * Add more test cases for deduplication * Extend the existing collector test to include new data for deduplication * Expand on test cases * Fix some failing tests * Fix invalid collector * More cleanup of the tests and regenerated data * Fix failures of data_statuses assert * Fix just a minimal version of gather, making sure it doesn;t throw an exception * Add working gather test * Add new validation, testing each deduplication usecase in detail * Outline the test cases more clearly and add proper numbering * Add more facts into collection Will help to debug deduplication issues * Fix ordering for asserts * Update metrics_utility/automation_controller_billing/dataframe_engine/dataframe_jobhost_summary_usage.py Co-authored-by: Martin Hradil <mhradil@redhat.com> * Update metrics_utility/automation_controller_billing/dataframe_engine/dataframe_jobhost_summary_usage.py Co-authored-by: Martin Hradil <mhradil@redhat.com> * Update metrics_utility/automation_controller_billing/dedup/ccsp.py Co-authored-by: Martin Hradil <mhradil@redhat.com> * Change product_serial and machine_id to be realistic * Verify and fix first set of cases * Expand the data to be able to test all cases * Fix the NAT example * Fix case with 2 regions * Fix test case 3.3 * Extract facts fro serial safely if the fact key was missing, it was exploding * Tweak the unit test after adding dedup of also main_host * Remove test case 1.7, it's duplicate of 1.2 * Fix test order and cleanup desctiprion of case 5.1 * Fix wrong cases description --------- Co-authored-by: Martin Hradil <mhradil@redhat.com>

Ladas force-pushed the deduplication_tests branch 3 times, most recently from fe1b705 to 9bad36d Compare July 13, 2025 15:22

Ladas marked this pull request as draft July 13, 2025 22:13

Ladas force-pushed the deduplication_tests branch 3 times, most recently from 03602b8 to ec1408a Compare July 14, 2025 09:01

Ladas marked this pull request as ready for review July 14, 2025 09:55

Ladas force-pushed the deduplication_tests branch 7 times, most recently from 7480119 to 7850a3f Compare July 17, 2025 08:37

himdel approved these changes Jul 21, 2025

View reviewed changes

AlexSCorey approved these changes Jul 22, 2025

View reviewed changes

Ladas force-pushed the deduplication_tests branch from 2dfd7be to 15c03ff Compare July 24, 2025 14:20

Ladas added 11 commits July 24, 2025 16:21

Add integration test testing new deduplication

c17ff29

Fix dedup in invetory_scope

7ce1687

Fix current test failing on new column thatshould show only with dedup

fb6acdc

Refactoring of handling host names before dedup

d4b7b89

Refactor host_names before dedup

d63c2a3

Organize helper functions better

f55e782

Add instructions on how to run tests in CI mode

26862ed

Current approach is hacky, but it doesn;t require any code change. We'll need to unify the CI run and the local run, or create another docker compose.

Test properly all sheets as dict comparissions

36ceb7f

Simplify host_names_before_decup

57f57b8

Fix host before dedup and mock of report date

546f4c5

Add more test cases for deduplication

f16c3a3

Ladas and others added 14 commits July 24, 2025 16:21

Fix some failing tests

d47ebe3

Fix invalid collector

632b06a

More cleanup of the tests and regenerated data

a9ba18e

Fix failures of data_statuses assert

c69c83b

Fix just a minimal version of gather, making sure it doesn;t throw an…

6ebee67

… exception

Add working gather test

bb9ee70

Add new validation, testing each deduplication usecase in detail

35edb95

Outline the test cases more clearly and add proper numbering

7946ab0

Add more facts into collection

b60a9e7

Will help to debug deduplication issues

Fix ordering for asserts

4f735d5

Update metrics_utility/automation_controller_billing/dataframe_engine…

1aec56e

…/dataframe_jobhost_summary_usage.py Co-authored-by: Martin Hradil <mhradil@redhat.com>

Update metrics_utility/automation_controller_billing/dataframe_engine…

1d1cafd

…/dataframe_jobhost_summary_usage.py Co-authored-by: Martin Hradil <mhradil@redhat.com>

Update metrics_utility/automation_controller_billing/dedup/ccsp.py

d2d0ba2

Co-authored-by: Martin Hradil <mhradil@redhat.com>

Change product_serial and machine_id to be realistic

07daa4c

Ladas force-pushed the deduplication_tests branch from 15c03ff to 07daa4c Compare July 24, 2025 14:36

Ladas added 9 commits July 24, 2025 17:32

Verify and fix first set of cases

dbd670a

Expand the data to be able to test all cases

6cc250d

Fix the NAT example

e0d0c41

Fix case with 2 regions

8a16654

Fix test case 3.3

3eee236

Extract facts fro serial safely

553b4a8

if the fact key was missing, it was exploding

Tweak the unit test after adding dedup of also main_host

12c8be3

Remove test case 1.7, it's duplicate of 1.2

9cb2fe0

Fix test order and cleanup desctiprion of case 5.1

0311fa4

himdel reviewed Jul 25, 2025

View reviewed changes

Fix wrong cases description

3a7f9fe

himdel merged commit afc880a into ansible:devel Jul 25, 2025
2 checks passed


		billing_data['serial'] = billing_data.apply(compute_serial, axis=1)

		experimental_dedup = self.extra_params.get('deduplicator') == 'ccsp-experimental'

Conversation

Ladas commented Jul 11, 2025 • edited by himdel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

himdel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud Bot commented Jul 21, 2025

Uh oh!

AlexSCorey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ladas Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Jul 25, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ladas commented Jul 11, 2025 •

edited by himdel

Loading

Ladas Jul 24, 2025 •

edited

Loading