Feature/disk stats metrics #12

stevanbz · 2023-05-23T20:31:34Z

Is your feature request related to a problem? Please provide an existing Issue # , or describe.
Added cpu/disk related metrics from performance-analyzer-rca project needed for distributed performance tracing.
Client (user of the commons lib) in order to be able to use the metrics, will have to add the permissions (described in src/main/resources/plugin-security.policy).
Copy-pasted code base for getting the native thread id, code for getting the diskIO, cpu and sched metrics.

Publishing to local maven is useful once you want to test the usage of the lib:
.

./gradlew clean build publishToMavenLocal

and then usage the lib as a dependency (if there is a need - exclude the clashing the lib's or maybe we should align the dependencies in this project according to the versions defined in Open search - check it out here):

api ("org.opensearch:performance-analyzer-commons:3.0.0.0-SNAPSHOT") {
    exclude (group: 'com.fasterxml.jackson.core', module: 'jackson-core')
    exclude (group: 'com.fasterxml.jackson.core', module: 'jackson-annotations')
    exclude (group: 'com.fasterxml.jackson', module: 'jackson-bom')
    exclude (group: 'com.fasterxml.jackson.core', module: 'jackson-databind')
  }

Edited a gradle task for publishing - since we had 2 tasks - one was publishing the jar to local mvn while the second one published only javadoc and sources jar.

Describe the solution you are proposing
A clear and concise description of what you want to happen.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Tjofil · 2023-05-24T15:33:03Z

Looks good to me.

Signed-off-by: Stevan Buzejic <[email protected]>

khushbr · 2023-05-24T21:40:08Z

src/main/java/org/opensearch/performanceanalyzer/commons/jvm/ThreadList.java

+        jTidNameMap.put(id, name);
+    }
+
+    static void runThreadDump(String pid, String[] args) {


Let us add a method javadoc highlighting: A thread dump is a snapshot of the state of all the threads of a Java process.

We use HotSpotVirtualMachine to capture thread dump. VirtualMachine is type cast to HotSpotVirtualMachine to access the method at runtime.

Use this method ONLY when NativeThreadId is required, for all other purpose us ThreadMxBeans API’s.

Running Thread Dump is expensive and should be only judiciously done.

khushbr · 2023-05-24T21:42:32Z

src/main/resources/plugin-security.policy

@@ -0,0 +1,26 @@
+/*


Let us rename this file, plugin-security isn't relevant for this library.

khushbr · 2023-05-24T21:43:18Z

...org/opensearch/performanceanalyzer/commons/metrics_generator/CPUPagingActivityGenerator.java

+    // This method will be called before all following get methods
+    // to make sure that all information exists for a thread id


Block comments are preferred.

khushbr · 2023-05-24T21:55:27Z

src/main/java/org/opensearch/performanceanalyzer/commons/os/ThreadCPU.java

+        }
+    }
+
+    public synchronized void addSample() {


JavaDoc for this new method. Additionally, we need 2 service timers - to measure the time taken for parsing, calculateCPUDetails and calculatePagingActivity

khushbr · 2023-05-24T21:56:03Z

src/main/java/org/opensearch/performanceanalyzer/commons/os/ThreadCPU.java

+        kvTimestamp = System.currentTimeMillis();
+        for (String tid : tids) {
+            Map<String, Object> sample =
+                    // (new SchemaFileParser("/proc/"+tid+"/stat",


Remove the comment ?

khushbr · 2023-05-24T22:02:37Z

src/main/java/org/opensearch/performanceanalyzer/commons/os/ThreadCPU.java

+    public synchronized void addSample(String threadId) {
+        tids = OSGlobals.getTids();
+
+        oldtidKVMap.clear();
+        oldtidKVMap.putAll(tidKVMap);
+
+        tidKVMap.clear();
+        oldkvTimestamp = kvTimestamp;
+        kvTimestamp = System.currentTimeMillis();
+        Map<String, Object> sample =
+                // (new SchemaFileParser("/proc/"+tid+"/stat",
+                (new SchemaFileParser(
+                                "/proc/" + pid + "/task/" + threadId + "/stat",
+                                statKeys,
+                                statTypes,
+                                true))
+                        .parse();
+        tidKVMap.put(threadId, sample);
+
+        calculateCPUDetails();
+        calculatePagingActivity();
+    }


Why do we need repeated logic here? Update public synchronized void addSample() { for parameters with default set as All thread.

khushbr · 2023-05-24T22:11:39Z

src/main/java/org/opensearch/performanceanalyzer/commons/os/ThreadSched.java

+    public synchronized void addSample() {
+        tids = OSGlobals.getTids();
+
+        oldtidKVMap.clear();
+        oldtidKVMap.putAll(tidKVMap);
+
+        tidKVMap.clear();
+        oldkvTimestamp = kvTimestamp;
+        kvTimestamp = System.currentTimeMillis();
+        for (String tid : tids) {
+            Map<String, Object> sample =
+                    (new SchemaFileParser(
+                                    "/proc/" + pid + "/task/" + tid + "/schedstat",
+                                    schedKeys,
+                                    schedTypes))
+                            .parse();
+            tidKVMap.put(tid, sample);
+        }
+
+        calculateSchedLatency();
+    }
+
+    public synchronized void addSample(String threadId) {
+        tids = OSGlobals.getTids();
+
+        oldtidKVMap.clear();
+        oldtidKVMap.putAll(tidKVMap);
+
+        tidKVMap.clear();
+        oldkvTimestamp = kvTimestamp;
+        kvTimestamp = System.currentTimeMillis();
+        Map<String, Object> sample =
+                (new SchemaFileParser(
+                                "/proc/" + pid + "/task/" + threadId + "/schedstat",
+                                schedKeys,
+                                schedTypes))
+                        .parse();
+        tidKVMap.put(threadId, sample);
+
+        calculateSchedLatency();
+    }


Let us not repeat code, can we refactor this to:

private static final ALL_THREADS = "ALL_THREADS"; public synchronized void addSampleForThread(String threadId) { addSample(threadId); } public synchronized void addSample(String threadId = ALL_THREADS) { ... }

Hm I am afraid we can't provide default value for the method parameter in Java (like we can do in Kotlin for example).

khushbr · 2023-05-24T22:15:52Z

src/main/java/org/opensearch/performanceanalyzer/commons/os/ThreadDiskIO.java

+    public static synchronized void addSample() {
+        tids = OSGlobals.getTids();
+        oldtidKVMap.clear();
+        oldtidKVMap.putAll(tidKVMap);
+
+        tidKVMap.clear();
+        oldkvTimestamp = kvTimestamp;
+        kvTimestamp = System.currentTimeMillis();
+        for (String tid : tids) {
+            addSampleTid(tid);
+        }
+    }
+
+    public static synchronized void addSample(String threadId) {
+        tids = OSGlobals.getTids();
+        oldtidKVMap.clear();
+        oldtidKVMap.putAll(tidKVMap);
+
+        tidKVMap.clear();
+        oldkvTimestamp = kvTimestamp;
+        kvTimestamp = System.currentTimeMillis();
+        addSampleTid(threadId);
+        for (String tid : tids) {
+            addSampleTid(tid);
+        }
+    }


Same comment as other place, instead of replicating code, let us refactor this to use method overloading.

Also, move the Map clear and load part, timestamp update to the private static void addSampleTid(String tid)

yeah - I just copy pasted the methods without too much thinking since my focus was mostly on the other side of the app - to make it workable and to be able to log the metrics for the given threadId. In this case, instead of adding samples for all the threads this method should add the sample for the given thread id:

public static synchronized void addSample(String threadId) { tids = OSGlobals.getTids(); oldtidKVMap.clear(); oldtidKVMap.putAll(tidKVMap); tidKVMap.clear(); oldkvTimestamp = kvTimestamp; kvTimestamp = System.currentTimeMillis(); addSampleTid(threadId); }

Yeah this method was just copy paste it should be something like:

public static synchronized void addSample(String threadId) { tids = OSGlobals.getTids(); oldtidKVMap.clear(); oldtidKVMap.putAll(tidKVMap); tidKVMap.clear(); oldkvTimestamp = kvTimestamp; kvTimestamp = System.currentTimeMillis(); addSampleTid(threadId); }

khushbr · 2023-05-24T22:34:49Z

...rch/performanceanalyzer/commons/metrics_generator/linux/LinuxCPUPagingActivityGenerator.java

+    private Map<String, Double> cpu;
+    private Map<String, Double[]> pagingActivities;


Can this made part of the interface CPUPagingActivityGenerator ? A more appropriate name for CPUPagingActivityGenerator is CPUPagingActivityStore.

Re the first question I didn't get that. If you can give me some more context it would be good. Tnx.

Re the second - I was thinking, maybe you are right. But the thing is that metrics generators are part of the the metrics_generator package and also, beside this, we have DiskIOMetricsGenerator, SchedMetricsGenerator etc. so if we decide to rename this class I have a feeling that we should be consistent for all other classes meaning - we should rename all of the ***Generators (since they are doing similar things - gathering the metrics and storing them in some data structures), or just keep them as they are.

What do you think? Tnx in advance!

khushbr · 2023-05-24T22:39:43Z

Couple of high level comments:

Naming Convention suggestion: Instead of using *ActivityGenerator, we should rename the interfaces to more apt *MetricStore after moving the HashMap here instead of the class.
Service stats are missing here, I am working on a separate PR and will add them.
There are no Unit Tests in this PR. I expect a follow-up PR to add these tests.

khushbr · 2023-05-25T18:30:32Z

src/main/java/org/opensearch/performanceanalyzer/commons/os/ThreadCPU.java

+        oldtidKVMap.clear();
+        oldtidKVMap.putAll(tidKVMap);
+
+        tidKVMap.clear();
+        oldkvTimestamp = kvTimestamp;
+        kvTimestamp = System.currentTimeMillis();
+        Map<String, Object> sample =
+                // (new SchemaFileParser("/proc/"+tid+"/stat",
+                (new SchemaFileParser(
+                                "/proc/" + pid + "/task/" + threadId + "/stat",
+                                statKeys,
+                                statTypes,
+                                true))
+                        .parse();
+        tidKVMap.put(threadId, sample);


Let us not clear the whole map when adding sample single threadID

…PU and DiskIO samples Signed-off-by: Stevan Buzejic <[email protected]>

Signed-off-by: Stevan Buzejic <[email protected]>

stevanbz · 2023-06-01T13:54:52Z

Closing the PR in favor of
#31

We decided not to go with adding the sample per one thread. Instead, we decided to have re-usable units of code that will be responsible for getting the metrics for the threads

stevanbz requested review from ansjcy, getsaurabh02, goyamegh, khushbr, praveensameneni, sbcd90 and sgup432 as code owners May 23, 2023 20:31

stevanbz mentioned this pull request May 24, 2023

Feature/main risabh otel stevanbz/OpenSearch#1

Draft

6 tasks

stevanbz added 3 commits May 24, 2023 21:49

Added disk stats related metrics

f87ce55

Signed-off-by: Stevan Buzejic <[email protected]>

Extended DiskIO with additional metrics

2acd2bf

Signed-off-by: Stevan Buzejic <[email protected]>

Added control ManagementPermission

96affd6

Signed-off-by: Stevan Buzejic <[email protected]>

stevanbz force-pushed the feature/disk-stats-metrics branch from b2c5a9a to 96affd6 Compare May 24, 2023 19:56

khushbr reviewed May 24, 2023

View reviewed changes

khushbr reviewed May 25, 2023

View reviewed changes

stevanbz added 3 commits May 25, 2023 22:34

Adjusted code according to comments. Refactor method for adding the C…

3c41511

…PU and DiskIO samples Signed-off-by: Stevan Buzejic <[email protected]>

Refactored ThredSched

4d7b5b2

Signed-off-by: Stevan Buzejic <[email protected]>

removed comment

5e6f57d

Signed-off-by: Stevan Buzejic <[email protected]>

stevanbz closed this Jun 1, 2023

		// This method will be called before all following get methods
		// to make sure that all information exists for a thread id

		private Map<String, Double> cpu;
		private Map<String, Double[]> pagingActivities;

Feature/disk stats metrics #12

Feature/disk stats metrics #12

Uh oh!

Conversation

stevanbz commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Check List

Uh oh!

Tjofil commented May 24, 2023

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!

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!

stevanbz May 25, 2023 • 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!

khushbr commented May 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevanbz commented Jun 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stevanbz commented May 23, 2023 •

edited

Loading

stevanbz May 25, 2023 •

edited

Loading