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.

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

Protocol Specific Network Metrics - Iteration 2 #15541

Conversation

@puzpuzpuz
Copy link
Contributor

commented Sep 12, 2019

  • Bytes send/received metrics for each ProtocolType are added as metrics and logged to diagnostics (only total, not per-thread or per-pipeline).
  • Bytes send/received are reported to Management Center.
  • Stats are exposed in NetworkingService as it seems to be the most logic place for them at the moment. If more stats arise in future, a separate class might be introduced to handle all of them.

Note that this works only when advanced networking is enabled and an endpoint configuration is made for the specific protocol type (member, client, WAN etc.).

MC PR: hazelcast/management-center#2062

Previous iteration: #15486

@puzpuzpuz puzpuzpuz requested review from emre-aydin and pveentjer Sep 12, 2019
@puzpuzpuz puzpuzpuz marked this pull request as ready for review Sep 12, 2019
… calls in TcpIpNetworkingService#start
@puzpuzpuz puzpuzpuz force-pushed the puzpuzpuz:v3.12/enhancement/protocol-specific-network-stats branch from 846b7a1 to cbc5036 Sep 13, 2019
@puzpuzpuz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

@pveentjer could you review this PR on this week?

@@ -175,6 +186,14 @@ public synchronized void start() {

networking.start();
startAcceptor();

if (advancedNetworkingEnabled) {

This comment has been minimized.

Copy link
@pveentjer

pveentjer Sep 23, 2019

Member

I need to think about this part.

This comment has been minimized.

Copy link
@puzpuzpuz

puzpuzpuz Sep 23, 2019

Author Contributor

The idea is that users who don't have advanced network stats enabled shouldn't "pay" for them. It doesn't make sense to spend CPU cycles on collection of these stats, as we know that they're going to be zeros.

This comment has been minimized.

Copy link
@pveentjer

pveentjer Sep 23, 2019

Member

Should this not always be enabled by default?

Also: the dynamic probe discovery stuff is pending.

This comment has been minimized.

Copy link
@puzpuzpuz

puzpuzpuz Sep 23, 2019

Author Contributor

Should this not always be enabled by default?

As far as I know, you need to enable advanced network config explicitly: https://docs.hazelcast.org/docs/3.12.2/manual/html-single/index.html#advanced-network-configuration

It makes sense that it's not enabled by default as advanced network config is about using separate ports for client/member/WAN/REST/memcache protocols.

This comment has been minimized.

Copy link
@puzpuzpuz

puzpuzpuz Sep 23, 2019

Author Contributor

Also: the dynamic probe discovery stuff is pending.

I'm not sure how it relates with this PR. Maybe you mean metrics redesign task? If that's the case, then you should check #15560

Copy link
Contributor

left a comment

Most of the work looks good to me.
I am a bit reluctant with the placement of the accumulator variables and therefore the logic & API changes due to that placement.
How about you make this change EndpointManager specific? Each endpoint knows its type, and its channels, so you can know in/out per endpoint. This helps with if (advancedIsEnabled) because the logic will be part of the particular endpoint, you shouldn't care on that layer whether it is advanced or not etc. Furthermore the Member will not have to expose public API for in/out stats, but you can access those directly through an endpoint per type, or aggregated from the AggregateEndpointManager. ie. Member is the wrong place to access network stats for all channels, its confusing, is it member only, client too? REST also? etc.

Last, if the stats become part of the EndpointManager then you can loose the prefix Advanced, it will also be implementation specific.

@puzpuzpuz puzpuzpuz force-pushed the puzpuzpuz:v3.12/enhancement/protocol-specific-network-stats branch from e138e5b to 080cf87 Sep 23, 2019
…ager

* Fix closed connections problem (stats weren't including counters from closed connections/channels)
* Add integration test that covers closed connection case
@puzpuzpuz puzpuzpuz force-pushed the puzpuzpuz:v3.12/enhancement/protocol-specific-network-stats branch from 080cf87 to 6ac928b Sep 24, 2019
@puzpuzpuz puzpuzpuz requested review from tkountis, emre-aydin and pveentjer Sep 24, 2019
@puzpuzpuz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

I have pushed changes that address all comments + one issue that wasn't solved in the initial implementation. Namely, per protocol stats weren't including counters for closed connections/channels, thus their counters weren't monotonic. This issue should be fixed now and covered with a new test.

Copy link
Contributor

left a comment

Thanks for the changes. LGTM
Minor comments which I forwarded to @puzpuzpuz in-chat for future discussion.

@puzpuzpuz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

Leaving a follow-up note with potential enhancements in future as the result of chat with @tkountis.

We may convert #calculate* methods in EMs into something like getNetStats() (with a couple of counters for bytes received/sent). Each of these NetStats objects will be updated individually with a metrics task. Then AggregateEM will return a Map<EndpointQualifier, NetStats>.

This API would be more flexible and feel more natural.

@puzpuzpuz puzpuzpuz merged commit 5829517 into hazelcast:maintenance-3.x Sep 25, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@puzpuzpuz puzpuzpuz deleted the puzpuzpuz:v3.12/enhancement/protocol-specific-network-stats branch Sep 25, 2019
puzpuzpuz added a commit to puzpuzpuz/hazelcast that referenced this pull request Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.