[GitHub] [ignite] agoncharuk commented on a change in pull request #7693: IGNITE-12666 Provide cluster performance profiling tool

[GitBox <gi...@apache.org>]

agoncharuk commented on a change in pull request #7693:
URL: https://github.com/apache/ignite/pull/7693#discussion_r436546495



##########
File path: modules/profiling/src/main/java/org/apache/ignite/internal/profiling/handlers/CacheOperationsHandler.java
##########
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.profiling.handlers;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.node.ArrayNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.UUID;
+import org.apache.ignite.internal.util.typedef.internal.U;
+
+import static org.apache.ignite.internal.profiling.ProfilingFilesParser.currentNodeId;
+import static org.apache.ignite.internal.profiling.util.Utils.MAPPER;
+import static org.apache.ignite.internal.profiling.util.Utils.createArrayIfAbsent;
+import static org.apache.ignite.internal.profiling.util.Utils.createObjectIfAbsent;
+
+/**
+ * Builds JSON with aggregated cache operations statistics.
+ *
+ * Example:
+ * <pre>
+ * {
+ *    $nodeId : {
+ *       $cacheId : {
+ *          $opType : [ [ $startTime, $count] ]
+ *       }
+ *    }
+ * }
+ * </pre>
+ */
+public class CacheOperationsHandler implements IgniteProfilingHandler {
+    /** Field name of aggregated by caches/nodes values. */
+    private static final String TOTAL = "total";
+
+    /** Cache operations statistics: nodeId->cacheId->opType->aggregatedResults. */
+    private final Map<UUID, Map<Integer, Map<String, Map<Long, Integer>>>> res = new HashMap<>();

Review comment:
       Such nested maps are very hard to read, please introduce separate classes for internal maps.

##########
File path: modules/profiling/src/main/java/org/apache/ignite/internal/profiling/util/ProfilingDeserializer.java
##########
@@ -0,0 +1,294 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.profiling.util;
+
+import java.nio.ByteBuffer;
+import java.util.UUID;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.ignite.internal.processors.cache.query.GridCacheQueryType;
+import org.apache.ignite.internal.profiling.IgniteProfiling;
+import org.apache.ignite.internal.profiling.IgniteProfiling.CacheOperationType;
+import org.apache.ignite.internal.profiling.LogFileProfiling;
+import org.apache.ignite.internal.profiling.LogFileProfiling.OperationType;
+import org.apache.ignite.internal.util.GridIntList;
+import org.apache.ignite.lang.IgniteUuid;
+
+import static org.apache.ignite.internal.profiling.LogFileProfiling.readIgniteUuid;
+import static org.apache.ignite.internal.profiling.LogFileProfiling.readUuid;
+
+/**
+ * Profiling operations deserializer.
+ *
+ * @see LogFileProfiling
+ */
+public class ProfilingDeserializer implements AutoCloseable {
+    /** Cached strings by id. */
+    private final ConcurrentHashMap<Short, String> stringById = new ConcurrentHashMap<>();
+
+    /** Handlers to process deserialized operation. */
+    private final IgniteProfiling[] handlers;
+
+    /** @param handlers Handlers to process deserialized operation. */
+    public ProfilingDeserializer(IgniteProfiling... handlers) {
+        this.handlers = handlers;
+    }
+
+    /**
+     * Tries to deserialize profiling operation from buffer.
+     *
+     * @param buf Buffer.
+     * @return {@code True} if operation parsed. {@code False} if not enough bytes.
+     */
+    public boolean deserialize(ByteBuffer buf) {
+        int pos = buf.position();
+
+        if (buf.remaining() < 1)
+            return false;
+
+        byte opTypeByte = buf.get();
+
+        OperationType opType = OperationType.fromOrdinal(opTypeByte);
+
+        switch (opType) {
+            case CACHE_OPERATION: {
+                if (buf.remaining() < 1 + 4 + 8 + 8)

Review comment:
       Such calculations are present at least in LogFileProfiling too, need to extract them to a single place to avoid errors.

##########
File path: bin/profiling.sh
##########
@@ -0,0 +1,130 @@
+#!/usr/bin/env bash

Review comment:
       What is the reason of putting this into a separate file? There is an activity on the dev-list to extend control.sh to be able to support pluggable commands. I think it makes sense to include profiling control to control.sh instead of a separate file.

##########
File path: modules/profiling/src/main/java/org/apache/ignite/internal/profiling/util/ProfilingDeserializer.java
##########
@@ -0,0 +1,294 @@
+/*

Review comment:
       Need to add a test that validates serializing-deserializing of the operations. There is no need to start Ignite, just an instance of LogFileProfiling writing all possible operations and then an instance of ProfilingDeserializer reading those logged operations.

##########
File path: modules/profiling/report/index.html
##########
@@ -0,0 +1,188 @@
+<!DOCTYPE html>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements.  See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License.  You may obtain a copy of the License at
+
+      http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<html lang="en">
+<head>
+    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
+    <meta name="theme-color" content="#563d7c">
+
+    <title>Ignite profiling report</title>
+
+    <!-- Bootstrap CSS. -->
+    <link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.5.0/css/bootstrap.min.css"
+          integrity="sha384-9aIt2nRpC12Uk9gS9baDl411NQApFmC26EwAOH8WgZl5MYYxFfc+NcPb1dKGj7Sk"
+          crossorigin="anonymous">
+
+    <!-- Bootstrap table. -->
+    <link rel="stylesheet" href="https://unpkg.com/bootstrap-table@1.16.0/dist/bootstrap-table.min.css">
+
+    <!-- Bootstrap select. -->
+    <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/bootstrap-select@1.13.14/dist/css/bootstrap-select.min.css">
+
+    <!-- Feather Icons. -->
+    <link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.6.3/css/all.css"
+          integrity="sha384-UHRtZLI+pbxtHCWp1t77Bi1L4ZtiqrqD80Kn4Z8NTSRyMA2Fd33n5dQ8lWUE00s/"
+          crossorigin="anonymous">

Review comment:
       This relates to .js as well. 
   The report will not work without internet connection. We should consider either dropping external dependencies or packing them in tool resources so it can generate a standalone self-sufficient report.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org