IGNITE-12108 TCP Communication Metrics ported to a new framework. by ibessonov · Pull Request #6814 · apache/ignite

ibessonov · 2019-08-27T08:48:32Z

No description provided.

nizhikov · 2019-09-11T13:49:06Z

+     * @param mreg Metrics registry.
     */
-    protected GridAbstractCommunicationClient(int connIdx, @Nullable GridNioMetricsListener metricsLsnr) {
+    protected GridAbstractCommunicationClient(int connIdx, @Nullable MetricRegistry mreg) {


Why mreg can be null?

This class might be used by components other than TCP SPI. You can see that "GridNioMetricsListener" was nullable as well.

nizhikov · 2019-09-11T13:54:22Z

        IgniteBiInClosure<GridNioSession, Integer> msgQueueLsnr,
        boolean readWriteSelectorsAssign,
        @Nullable GridWorkerListener workerLsnr,
+        @Nullable MetricRegistry mreg,


Why mreg can be null?

Same as above.

Let me please argue with you about this particular constructor =)
I totally forgot that it's a private constructor and only "GridNioServer.Builder#build" invokes it. There's no point in splitting it because no one will invoke other constructor with fewer parameters. Let's leave it as is.

Just in case - GridAbstractCommunicationClient constructor will be fixed as you asked. I't not about arguing for the sake of arguing.

Thanks, Ilya.
Please, let me know when PR becomes ready to be reviewed.

nizhikov · 2019-09-11T13:55:08Z

+            return -1;

-        return res;
+        return (int) mreg.longAdderMetric(


We should store LongMetric inside object instance.

I don't get it, can you please explain this comment in more details? Is this about excessive memory allocation?

It's about unnecessary Map#get operation.
Every 'mreg.longAdderMetrictranslates toMap#get`.
Why do we need it? We can store the variable, isn't it?

nizhikov · 2019-09-11T14:02:06Z

+            RECEIVED_MESSAGES_BY_TYPE_METRIC_DESC
+        );
+
+        sentMsgsCntByNodeIdMetricFactory = nodeId -> mmgr.registry(COMMUNICATION_METRICS_GROUP_NAME + "." + nodeId)


Let's use MetricUtils#metricName instead of COMMUNICATION_METRICS_GROUP_NAME + "." + nodeId

nizhikov · 2019-09-11T14:03:39Z

    public static final short HANDSHAKE_WAIT_MSG_TYPE = -28;

+    /** Communication metrics group name. */
+    public static final String COMMUNICATION_METRICS_GROUP_NAME = "communication.tcp";


Let's use MetricUtils#metricName instead of "communication.tcp".

nizhikov · 2019-09-11T14:05:49Z

+
+
+    /** */
+    public static String sentMessagesByTypeMetricName(Short directType) {


Let's declare all constants, first, and methods after.

nizhikov · 2019-09-11T14:06:14Z

+
+    /** */
+    public static String sentMessagesByTypeMetricName(Short directType) {
+        return "sentMessagesByType." + directType;


Let's use MetricUtils#metricName instead of this and similar methods.

nizhikov · 2019-09-11T14:07:31Z

-        this.log = rootLog.getLogger(getClass());
-        this.jmx = prepareMBeanServer();
-        this.rsrcProc = new GridResourceProcessor(new GridTestKernalContext(this.log, this.cfg));
+        log = rootLog.getLogger(getClass());


why this change?

"this." was not necessary so I removed it while adding new field.

Please, revert unnecessary changes.

nizhikov · 2019-09-11T14:07:35Z

        this.jmx = jmx;
-        this.log = rootLog.getLogger(getClass());
-        this.rsrcProc = new GridResourceProcessor(new GridTestKernalContext(this.log));
+        log = rootLog.getLogger(getClass());


why this change?

same as above.

Please, revert unnecessary changes.

nizhikov · 2019-09-11T14:07:41Z

        this.log = log.getLogger(getClass());
-        this.jmx = prepareMBeanServer();
-        this.rsrcProc = new GridResourceProcessor(new GridTestKernalContext(this.log));
+        jmx = prepareMBeanServer();


why this change?

Please, revert unnecessary changes.

nizhikov · 2019-11-22T12:29:44Z

            new GridResourceLoggerInjector(ctx.config().getGridLogger());
        injectorByAnnotation[GridResourceIoc.ResourceAnnotation.IGNITE_INSTANCE.ordinal()] =
            new GridResourceBasicInjector<>(ctx.grid());
+        injectorByAnnotation[GridResourceIoc.ResourceAnnotation.METRIC_MANAGER.ordinal()] =


Why do we need new GridResourceSupplierInjector?
Can we use existing GridResourceBasicInejctor?

Because supplier injector is lazy. In this particular place metric manager might not be initialized yet.

nizhikov · 2019-11-22T12:31:49Z


        this.formatter = formatter;
+
+        sentBytesCntMetric = mreg == null ?


mreg can't be null here.

Good catch, thank you!

nizhikov · 2019-11-22T12:39:41Z

     * Gets sent messages count.
     *
     * @return Sent messages count.
+     * @deprecated Will be removed in the next major release and replaced with new metrics API.


We should inform user - how he(or she) should obtain information previously gathered from deprecated method.

Let's write "Use metric 'xxx.yyy.zzz' instead" where xxx.yyy.zzz the name of the metric providing required number.

Please, apply this to other deprecated method comments.

I need you advice about the comment. Are there any plans to cancel JMX support for these values? I didn't think about it long enough. Maybe just remove that Deprecated annotation.
Anyway, specifying some metrics names here would be wrong, CommunicationSpi is interface after all and some implementations may want to use some other specific metric names, or no metrics at all.

nizhikov · 2019-11-22T12:52:06Z

+    }
+
+    /** */
+    public static String receivedMessagesByTypeMetricName(Short directType) {


Let's move this method to TcpCommunicationMetricsListener.
As I can see it used only there.

nizhikov · 2019-11-22T12:56:53Z

+    public static final String RECEIVED_MESSAGES_BY_NODE_ID_METRIC_DESC = "Total number of messages received by current node from the given node";
+
+    /** */
+    public static String sentMessagesByTypeMetricName(Short directType) {


Let's move this method to TcpCommunicationMetricsListener.
As I can see it used only there.

nizhikov · 2019-11-22T13:15:41Z

-        }
-    };
+    /** Sent bytes count metric.*/
+    private final AtomicLongMetric sentBytesMetric;


Let's keep LongAdder as an internal storage for all metrics that was based on it previously.
You can use MetricRegistry#longAdderMetric for it.

nizhikov · 2019-11-22T13:18:04Z


-    /** Sent bytes count.*/
-    private final LongAdder sentBytesCnt = new LongAdder();
+    /** */


Please, add javadoc for this variable.

nizhikov · 2019-11-22T13:19:26Z

-    /** All registered metrics. */
-    private final Set<ThreadMetrics> allMetrics = Collections.newSetFromMap(new ConcurrentHashMap<>());
+    /** */
+    private final Function<Short, AtomicLongMetric> rcvdMsgsCntByTypeMetricFactory;


Please, add javadoc for this variable.

nizhikov · 2019-11-22T13:19:30Z

-        @Override protected ThreadMetrics initialValue() {
-            ThreadMetrics metrics = new ThreadMetrics();
+    /** */
+    private final Function<Object, AtomicLongMetric> sentMsgsCntByConsistentIdMetricFactory;


Please, add javadoc for this variable.

nizhikov · 2019-11-22T13:21:22Z

+    ConcurrentHashMap<UUID, AtomicLong> rcvdMsgsMetricsByNodeId = new ConcurrentHashMap<>();
+
+    /** Sent messages count metrics grouped by message node consistent id. */
+    ConcurrentHashMap<Object, AtomicLongMetric> sentMsgsMetricsByConsistentId = new ConcurrentHashMap<>();


I'm not sure we should change the way we collect metrics in this PR.
Let's keep existing behaviour?

This also will simplify PR a lot.

I changed that because in old implementation we stored basically the same map in each thread separately. That's the obvious answer.

The other thing is that old implementation had concurrency issues that no one cared about. Like broken values visibility or potential ConcurrentModificationExceptions. I wanted new implementation to be correct.

You introduced metrics based on node consistency id.
I think we should avoid introducing new metrics here.

The only goal of this ticket is to migrate existing metrics to the new framework.

Let's

remove metrics based on consistency id.

revert changes in ConnectionKey and TcpCommunicationSpi that made to support it.

nizhikov · 2019-11-22T13:24:03Z

@ibessonov Thanks for the PR update.

Looks much better now!
Please, take a look at my comments

nizhikov · 2019-12-20T07:04:44Z

 import org.apache.ignite.plugin.extensions.communication.MessageFormatter;
+import org.jetbrains.annotations.Nullable;
+
+import static org.apache.ignite.internal.util.nio.GridNioServer.SENT_BYTES_METRIC_DESC;


nit: unused import.

nizhikov · 2019-12-20T07:11:02Z


    /**
     * Resets metrics for this SPI instance.
+     *


nit: unnecessary change.

nizhikov · 2019-12-20T15:29:06Z

@ibessonov

In TcpCommunicationMetricsListener let's inline the following variables. They used only once in the code:

    /** Function to be used in {@link Map#computeIfAbsent(Object, Function)} of {@link #sentMsgsMetricsByType}. */
    private final Function<Short, LongAdderMetric> sentMsgsCntByTypeMetricFactory;

    /** Function to be used in {@link Map#computeIfAbsent(Object, Function)} of {@link #rcvdMsgsMetricsByType}. */
    private final Function<Short, LongAdderMetric> rcvdMsgsCntByTypeMetricFactory;

    /** Function to be used in {@link Map#computeIfAbsent(Object, Function)} of {@link #sentMsgsMetricsByNodeId}. */
    private final Function<UUID, LongAdderMetric> sentMsgsCntByNodeIdMetricFactory;

    /** Function to be used in {@link Map#computeIfAbsent(Object, Function)} of {@link #rcvdMsgsMetricsByNodeId}. */
    private final Function<UUID, LongAdderMetric> rcvdMsgsCntByNodeIdMetricFactory;

The only reason we need a new annotation for a GridMetricManager injection is our tests. Can you, please, create a ticket to investigate and fix it. Then we can remove the new injector and use one that injects Ignite.

All other LGTM.
Please, rerun tests after fixin #1 and I will merge your PR.

Thanks!

nizhikov · 2019-12-24T10:07:16Z

@ibessonov As we discussed privately we should resolve potential contention on ConcurrentHashMap access.

            sentMsgsMetricsByType.computeIfAbsent(msg.directType(), sentMsgsCntByTypeMetricFactory).increment();

            sentMsgsMetricsByNodeId.computeIfAbsent(nodeId, sentMsgsCntByNodeIdMetricFactory).increment();

We also should perform a yardstick check of these changes.

nizhikov · 2020-01-09T16:54:24Z

+    private static synchronized MetricRegistry getOrCreateMetricRegistry(GridMetricManager mmgr, UUID nodeId) {
+        String regName = MetricUtils.metricName(COMMUNICATION_METRICS_GROUP_NAME, nodeId.toString());
+
+        for (MetricRegistry mreg : mmgr) {


We can eliminate iteration on all registries if we use GridMetricManager#addMetricRegistryCreationListener to implement registry initialization logic.

mmgr.addMetricRegistryCreationListener(mreg -> { if (!mreg.name().startsWith(COMMUNICATION_METRICS_GROUP_NAME)) return; mreg.longAdderMetric(SENT_MESSAGES_BY_NODE_ID_METRIC_NAME, SENT_MESSAGES_BY_NODE_ID_METRIC_DESC); mreg.longAdderMetric(RECEIVED_MESSAGES_BY_NODE_ID_METRIC_NAME, RECEIVED_MESSAGES_BY_NODE_ID_METRIC_DESC); });

nizhikov · 2020-01-09T16:59:20Z

+                            res.put(typeName, ((LongMetric)metric).value());
+                    }
+                }
+                catch (NumberFormatException ignore) {


Let's print the warning to log here.

I'll just remove it, it's excessive

nizhikov · 2020-01-09T17:00:25Z

+
+                    res.put(nodeId, mreg.<LongMetric>findMetric(metricName).value());
+                }
+                catch (IllegalArgumentException ignore) {


We shouldn't swallow exception here. Let's remove empty catch block.

…cks; metrics registration moved to listeners.

nizhikov · 2020-01-10T09:12:06Z

LGTM.

@ibessonov Thank you, so much for the valuable contribution!
I will gather performance tests results and merge this patch if everything is OK.

…#6814)

ibessonov added 2 commits August 27, 2019 11:46

IGNITE-12108 TCP Communication Metrics ported to a new framework.

56b4aba

IGNITE-12108 Checkstyle fixed.

6bb92fa

nizhikov reviewed Sep 11, 2019

View reviewed changes

ibessonov added 9 commits September 25, 2019 16:32

Merge remote-tracking branch 'origin/master' into ignite-12108

414d8d7

IGNITE-12108 nodeId replaced with consistentId for usability purposes

cc1145f

IGNITE-12108 Last review remarks.

967697d

IGNITE-12108 checkstyle

3a46712

Merge remote-tracking branch 'origin/master' into ignite-12108

1e652c5

IGNITE-12108 Review remarks addresses.

6886114

IGNITE-12108 one more change.

946557d

Merge remote-tracking branch 'origin/master' into ignite-12108

5a21644

IGNITE-12108 Extra fields removed from GridAbstractCommunicationClient.

76732cf

nizhikov requested changes Nov 22, 2019

View reviewed changes

ibessonov added 2 commits December 5, 2019 12:59

IGNITE-12108 More review remarks fixed.

80c643d

Merge remote-tracking branch 'origin/master' into ignite-12108

e96a936

nizhikov reviewed Dec 20, 2019

View reviewed changes

ibessonov added 2 commits December 20, 2019 15:20

IGNITE-12108 Minor code cleanup.

bc35d37

IGNITE-12108 REVERTED consistentId-based metrics.

ff87669

ibessonov added 3 commits December 20, 2019 18:49

IGNITE-12108 assertion for tests.

7d24eb1

IGNITE-12108 assertion removed.

d6fffb6

Merge branch 'apache-master' into ignite-12108

afa0e3b

ibessonov added 2 commits December 25, 2019 18:53

IGNITE-12108 Avoided lookups in concurrent hash maps on every message.

0aa0ea0

IGNITE-12108 HashMap replaced with Map in ThreadMetrics

002bd64

nizhikov approved these changes Jan 9, 2020

View reviewed changes

nizhikov reviewed Jan 9, 2020

View reviewed changes

Comment thread ...e/src/main/java/org/apache/ignite/spi/communication/tcp/TcpCommunicationMetricsListener.java

nizhikov reviewed Jan 9, 2020

View reviewed changes

ibessonov added 3 commits January 10, 2020 11:24

IGNITE-12108 Latest review remarks addressed: removed empty catch blo…

d93e7e5

…cks; metrics registration moved to listeners.

IGNITE-12108 Method inlined.

284bc63

IGNITE-12108 Minor refactoring.

f07eb73

nizhikov merged commit 1d97529 into apache:master Jan 14, 2020

nizhikov pushed a commit that referenced this pull request Jan 14, 2020

IGNITE-12108: TCP Communication Metrics migration to a new framework. (…

f7fcc40

…#6814)



		/** */
		public static String sentMessagesByTypeMetricName(Short directType) {


		this.formatter = formatter;

		sentBytesCntMetric = mreg == null ?

Uh oh!

Conversation

ibessonov commented Aug 27, 2019

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!

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!

nizhikov Sep 11, 2019 • 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!

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!

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!

nizhikov Sep 11, 2019 •

edited

Loading

ibessonov Dec 4, 2019 •

edited

Loading

nizhikov commented Dec 20, 2019 •

edited

Loading

nizhikov Jan 9, 2020 •

edited

Loading