Modifying logic to provide metrics per MINIO endpoint #83

nsarras · 2023-10-30T23:18:14Z

No description provided.

dss_metrics/minio_rest_collector.py

mssawant · 2023-11-07T01:31:24Z

@nsarras, why the commit message is empty? don't we follow any specific commit message template?

…n handling

nsarras · 2023-11-07T04:14:16Z

@nsarras, why the commit message is empty? don't we follow any specific commit message template?

I am not sure what you are referring to, each commit has a message

grandsuri

Each thread is having its own logger thread to log into their own files. This is not right way to do. We need to implement Queue handle based logger (you can borrow from DM repo) and pass the logger function handle to the thread.

if each thread is writing their log files, then someone has to go through all the files to debug

dss_metrics/metrics.py

grandsuri · 2023-11-07T06:49:59Z

dss_metrics/metrics.py

+
+    os.makedirs(os.path.dirname(logfile_path), exist_ok=True)
+    if not os.path.exists(logfile_path):
+        os.mknod(logfile_path)


os.mknod() is used to create /dev/ usually.
Instead of that you can do

with os.open(file_path, flags) as f: pass

mssawant · 2023-11-07T18:18:00Z

I am not sure what you are referring to, each commit has a message

@nsarras, I mean Modifying logic to provide metrics per MINIO endpoint doesn't actually give a clear picture of the problem that this PR is fixing. I mean why the earlier logic didn't work, is there a corresponding github issue or jira ticket that you can link this PR to? or is it just a refactoring PR. It is good to describe the problem and solution in a way that gives reviewer an idea what to expect from this PR.
Consider this format,

Header line: explain the commit in one line (use the imperative)

Body of commit message is a few lines of text, explaining things
in more detail, possibly giving some background about the issue
being fixed, etc.

The body of the commit message can be several paragraphs, and
please do proper word-wrap and keep columns shorter than about
74 characters or so. That way "git log" will show things
nicely even when it's indented.

Make sure you explain your solution and why you're doing what you're
doing, as opposed to describing what you're doing. Reviewers and your
future self can read the patch, but might not understand why a
particular solution was implemented.

Reported-by: whoever-reported-it
Signed-off-by: Your Name <[email protected]>

mssawant · 2023-11-07T18:30:35Z

dss_metrics/minio_rest_collector.py

+                            continue
+                        metrics_data_buffer.append(
+                            metrics.MetricInfo(
+                                metric[0], metric[0], metric[1], tags,


Why pass metrics selectively?

It is because we may not want to expose all metrics to the end user all of the time, hence why we filter using a whitelist if the filter flag is given

I mean, why not metrics.MetricInfo(metric, tags..) ? Let MetricInfo filter out.
metrics.MetricInfo(metric[0], metric[0], metric[1], tags, doen't really help readability, else its better to use named params.

See new commits on PR, I made a lot of changes and not using this approach anymore

nsarras · 2023-11-07T19:43:52Z

I am not sure what you are referring to, each commit has a message

@nsarras, I mean Modifying logic to provide metrics per MINIO endpoint doesn't actually give a clear picture of the problem that this PR is fixing. I mean why the earlier logic didn't work, is there a corresponding github issue or jira ticket that you can link this PR to? or is it just a refactoring PR. It is good to describe the problem and solution in a way that gives reviewer an idea what to expect from this PR. Consider this format,
Header line: explain the commit in one line (use the imperative)

Body of commit message is a few lines of text, explaining things
in more detail, possibly giving some background about the issue
being fixed, etc.

The body of the commit message can be several paragraphs, and
please do proper word-wrap and keep columns shorter than about
74 characters or so. That way "git log" will show things
nicely even when it's indented.

Make sure you explain your solution and why you're doing what you're
doing, as opposed to describing what you're doing. Reviewers and your
future self can read the patch, but might not understand why a
particular solution was implemented.

Reported-by: whoever-reported-it
Signed-off-by: Your Name <[email protected]>

@mssawant
Typically, we track tasks in JIRA; http://sj-jira.ssi.samsung.com:8080/browse/MIN-1876

I think you have a good suggestion and would be open to adopting that if we have the entire team agree that we want all of the details in each commit vs JIRA. The standard should be consistent across the team, and if you see other commits, the commit messages just indicate what was done, vs give a full summary & justification.

mssawant · 2023-11-07T22:34:21Z

@nsarras, good that you find my suggestion helpful. So, is this code open? if so, community does not have access to internal Jira servers.

Even though is the code is used for internal purposes, an elaborate commit message helps to give more defined information about the problem and corresponding fix to someone who is just looking at git history searching for a particular case.
One of the most important application of git history is auditing, it helps to expedite resolutions in case of patent conflicts as well.
Imagine commit messages as history of a particular product or a software. Without history you wouldn't know why a particular change happened and its significance.

…, add scope tagging for metrics, other bug fixes

sonarqubecloud · 2023-11-23T02:17:17Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

0.0% Coverage
0.0% Duplication

The version of Java (11.0.17) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

grandsuri

you can fix those issues if needed. otherwise looks good

grandsuri · 2023-11-28T22:20:57Z

dss_metrics/metrics.py

        while True:
            time.sleep(1)
    except Exception as error:
-        print(f"Failed to start Metrics http server: {str(error)}")
+        logger.info(f"Failed to start Metrics http server: {str(error)}")


you can call logger.exception to catch the backtrace

grandsuri · 2023-11-28T22:23:32Z

dss_metrics/minio_rest_collector.py

+
+                    name = key
+                    metric = Metric(name, key, 'gauge')
+                    metric.add_sample(


this kind of code just increases the lines decreasing the readability.
If the logic is too complicated, makes sense to have each lines for arguments

Modifying logic to provide metrics per MINIO endpoint

59a7a34

nsarras requested a review from a team as a code owner October 30, 2023 23:18

nsarras linked an issue Oct 30, 2023 that may be closed by this pull request

[Metrics Agent] Refactor DSS Metrics Agent to provide MINIO metrics per endpoint instead of per MINIO cluster #81

Closed

grandsuri reviewed Oct 31, 2023

View reviewed changes

dss_metrics/minio_rest_collector.py Outdated Show resolved Hide resolved

dss_metrics/minio_rest_collector.py Outdated Show resolved Hide resolved

dss_metrics/minio_rest_collector.py Show resolved Hide resolved

Adding logging with rotation after reaching 100MB. Improving exceptio…

ebe061c

…n handling

grandsuri reviewed Nov 7, 2023

View reviewed changes

mssawant reviewed Nov 7, 2023

View reviewed changes

nsarras added 3 commits November 21, 2023 12:47

Added logging, refactored agent to be multi-threaded vs multi-process…

c3101bf

…, add scope tagging for metrics, other bug fixes

Switching logging level

c1ebfaa

Refactoring to remove duplicate lines of code

1bddd1f

grandsuri approved these changes Nov 28, 2023

View reviewed changes

nsarras merged commit 4863a4c into master Nov 28, 2023
7 checks passed

nsarras deleted the 81-metrics-agent-refactor-dss-metrics-agent-to-provide-minio-metrics-per-endpoint-instead-of-per-minio-cluster branch November 28, 2023 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifying logic to provide metrics per MINIO endpoint #83

Modifying logic to provide metrics per MINIO endpoint #83

nsarras commented Oct 30, 2023

mssawant commented Nov 7, 2023

nsarras commented Nov 7, 2023

grandsuri left a comment

grandsuri Nov 7, 2023

mssawant commented Nov 7, 2023 •

edited

Loading

mssawant Nov 7, 2023

nsarras Nov 7, 2023

mssawant Nov 7, 2023

nsarras Nov 21, 2023

nsarras commented Nov 7, 2023

mssawant commented Nov 7, 2023

sonarqubecloud bot commented Nov 23, 2023

grandsuri left a comment

grandsuri Nov 28, 2023

grandsuri Nov 28, 2023

Modifying logic to provide metrics per MINIO endpoint #83

Modifying logic to provide metrics per MINIO endpoint #83

Conversation

nsarras commented Oct 30, 2023

mssawant commented Nov 7, 2023

nsarras commented Nov 7, 2023

grandsuri left a comment

Choose a reason for hiding this comment

grandsuri Nov 7, 2023

Choose a reason for hiding this comment

mssawant commented Nov 7, 2023 • edited Loading

mssawant Nov 7, 2023

Choose a reason for hiding this comment

nsarras Nov 7, 2023

Choose a reason for hiding this comment

mssawant Nov 7, 2023

Choose a reason for hiding this comment

nsarras Nov 21, 2023

Choose a reason for hiding this comment

nsarras commented Nov 7, 2023

mssawant commented Nov 7, 2023

sonarqubecloud bot commented Nov 23, 2023

grandsuri left a comment

Choose a reason for hiding this comment

grandsuri Nov 28, 2023

Choose a reason for hiding this comment

grandsuri Nov 28, 2023

Choose a reason for hiding this comment

mssawant commented Nov 7, 2023 •

edited

Loading