Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unify Process Values #5500

Merged
merged 11 commits into from
Jun 20, 2024
Merged

Unify Process Values #5500

merged 11 commits into from
Jun 20, 2024

Conversation

rafabailon
Copy link
Member

@rafabailon rafabailon commented Jun 14, 2024 •
edited
Loading

Description

Related Issue
#5479

Create a method in the DataVisualizer class that allows to unify the data obtained from the processes and subprocesses so that, when a plot is generated, the data of processes and subprocesses do not appear but all the data unified in a single process.

Testing performed

The tests can be performed locally using the DataVisualizer or using the CLUSTER-Workload_benchmarks_metrics pipeline with the simplest possible tests. It is only necessary to activate the option to unify data and provide 2 or more CSV of processes (one process and one or more sub processes).

Validation Jenkins Local OS
Check plots and painting information 🟢 🟢 Ubuntu 22.04

@rafabailon rafabailon linked an issue Jun 14, 2024 that may be closed by this pull request
Unify monitoring metrics values for child processes with those of the main process #5479
Closed
2 tasks
@rafabailon
Copy link
Member Author

Local Test

CSV Files
Steps 
git clone https:/wazuh/wazuh-qa.git
cd wazuh-qa
git checkout enhancement/5479-unify-process-values
python3 -m pip install -r requirements.txt
cd deps/wazuh_testing/
python3 setup.py install
cd wazuh_testing/scripts/
python3 data_visualizations.py -s wazuh-indexer.csv wazuh-indexer_child_1.csv -u True -t binary -d ~
Plots Generated

image

results.zip

@rafabailon rafabailon force-pushed the enhancement/5479-unify-process-values branch from 982d677 to a21ace0 Compare June 14, 2024 12:04
@rafabailon
Copy link
Member Author

Jenkins Test

Evidences

image

image

Note: No CSVs of subprocesses have been generated due to the simplicity of the test. However, it is possible to see that the command has used the -u parameter and the plot has been generated correctly.

@rafabailon rafabailon marked this pull request as ready for review June 14, 2024 14:40
Rebits
Rebits requested changes Jun 17, 2024
View reviewed changes
Copy link
Member

@Rebits Rebits left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed logic does not meet the issue requirements. It should unify the binary metrics of each daemon and its child processes. Currently, it unifies the metrics of all the daemons together.

image

deps/wazuh_testing/wazuh_testing/tools/performance/visualization.py Outdated
@@ -32,7 +32,7 @@ class DataVisualizer:
base_name (str, optional): base name used to store the images.
"""
def __init__(self, dataframes, target, compare=False, store_path=gettempdir(), x_ticks_granularity='minutes',
x_ticks_interval=1, base_name=None, columns_path=None):
x_ticks_interval=1, base_name=None, columns_path=None, unify=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
x_ticks_interval=1, base_name=None, columns_path=None, unify=False):
x_ticks_interval=1, base_name=None, columns_path=None, unify_child_daemon_metrics=False):

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solved in 4f12863

deps/wazuh_testing/wazuh_testing/tools/performance/visualization.py Outdated
Comment on lines 51 to 52
if unify.lower() in ["true"]:
self._unify_dataframes()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if unify.lower() in ["true"]:
self._unify_dataframes()
if unify_child_daemons_metrics:
self._unify_dataframes()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solved in 4f12863

@rafabailon
Copy link
Member Author

Update

I have fixed the variable name and changed the logic to unify only the correct daemons. I have yet to fix a bug in the groupby so that it groups by Daemon and Timestamp so that it does the correct counts.

@rafabailon
Copy link
Member Author

Update

The problem was that, when creating the dataframe, a grouping by Timestamp was established. I have corrected this to be able to correctly group the data and unify them. I have improved the unification conditional to avoid errors due to a bad use. I have also improved the logic with dataframe specific functions. I have taken the decision to keep and unify the PID and Version values since we can choose which columns to plot and which to discard. This avoids errors because if we do not discard those columns and unify, it would give error when plotting.

Local Test

CSV Files

csv_files.zip

Steps 
git clone https:/wazuh/wazuh-qa.git
cd wazuh-qa
git checkout enhancement/5479-unify-process-values
python3 -m pip install -r requirements.txt
cd deps/wazuh_testing/
python3 setup.py install
cd wazuh_testing/scripts/
python3 data_visualizations.py -s wazuh-apid.csv wazuh-apid_child_1.csv wazuh-apid_child_2.csv wazuh-analysisd.csv wazuh_clusterd.csv wazuh_clusterd_child_1.csv -u True -t binary -d ~
Plots Generated

image

image

plots.zip

@rafabailon
Copy link
Member Author

rafabailon commented Jun 18, 2024 •
edited
Loading

Jenkins Test

Evidences

image

image

@rafabailon rafabailon requested a review from Rebits June 18, 2024 12:19
Rebits
Rebits requested changes Jun 19, 2024
View reviewed changes
Copy link
Member

@Rebits Rebits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some final changes

deps/wazuh_testing/wazuh_testing/tools/performance/visualization.py Outdated Show resolved Hide resolved
deps/wazuh_testing/wazuh_testing/tools/performance/visualization.py Outdated Show resolved Hide resolved
deps/wazuh_testing/wazuh_testing/scripts/data_visualizations.py Outdated Show resolved Hide resolved
Rebits
Rebits previously approved these changes Jun 20, 2024
View reviewed changes
Copy link
Member

@Rebits Rebits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, gj

juliamagan
juliamagan requested changes Jun 20, 2024
View reviewed changes
Copy link
Member

@juliamagan juliamagan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GJ! Remember to add the changelog entry

@juliamagan juliamagan merged commit 0e07651 into 4.8.1 Jun 20, 2024
1 of 2 checks passed
@juliamagan juliamagan deleted the enhancement/5479-unify-process-values branch June 20, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliamagan juliamagan juliamagan approved these changes

@Rebits Rebits Rebits left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unify monitoring metrics values for child processes with those of the main process
3 participants
@rafabailon @Rebits @juliamagan