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

[API-537] Add support for the binary metrics #441

Merged
merged 2 commits into from
Aug 6, 2021

Conversation

mdumandag
Copy link
Contributor

Implemented the mechanism to compress the metrics we collect in a
way that is compatible with the server-side.

With this change, the latest Management Center versions are able to
show statistics regarding the Python client.

For backward compatibility, we also continue sending metrics in
a comma separated key-value format (client attributes).

In this PR, I also significantly cleaned up the statistics service.
In the old code, we were trying to reuse the results of some
calls. In the benchmarks I did, I saw that it didn't really improve
much. (New approach approx. 0.5 ms, old approach approx. 0.4 ms per
statistics collection) This really small difference does not justify
the need for complexity so I went with the simpler approach.

Also, aligned the log levels with the Java client for the various
warnings.

Also, bumped the version of psutil to 5.8.0.

@mdumandag
Copy link
Contributor Author

Here is the benchmark I used to compare the old vs new approach

import timeit
import psutil


def f1():
    psutil.virtual_memory().total
    psutil.virtual_memory().free
    psutil.virtual_memory().used
    psutil.swap_memory().total
    psutil.swap_memory().free
    psutil.cpu_percent()
    psutil.cpu_count()
    p = psutil.Process()
    p.memory_info().rss
    p.num_fds()
    p.rlimit(psutil.RLIMIT_NOFILE)[1]
    p.cpu_times()
    p.create_time()


def f2():
    v = psutil.virtual_memory()
    v.total
    v.free
    v.used
    s = psutil.swap_memory()
    s.total
    s.free
    psutil.cpu_percent()
    psutil.cpu_count()
    p = psutil.Process()
    with p.oneshot():
        p.memory_info().rss
        p.num_fds()
        p.rlimit(psutil.RLIMIT_NOFILE)[1]
        p.cpu_times()
        p.create_time()


print(timeit.timeit(f1, number=10000))
print(timeit.timeit(f2, number=10000))

Implemented the mechanism to compress the metrics we collect in a
way that is compatible with the server-side.

With this change, the latest Management Center versions are able to
show statistics regarding the Python client.

For backward compatibility, we also continue sending metrics in
a comma separated key-value format (client attributes).

In this PR, I also significantly cleaned up the statistics service.
In the old code, we were trying to reuse the results of some
calls. In the benchmarks I did, I saw that it didn't really improve
much. (New approach approx. 0.5 ms, old approach approx. 0.4 ms per
statistics collection) This really small difference does not justify
the need for complexity so I went with the simpler approach.

Also, aligned the log levels with the Java client for the various
warnings.

Also, bumped the version of psutil to 5.8.0.
@mdumandag mdumandag changed the title Add support for the binary metrics [API-537] Add support for the binary metrics Aug 4, 2021
@srknzl srknzl self-requested a review August 4, 2021 13:29
Copy link
Member

@srknzl srknzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good left some minor comments

hazelcast/metrics.py Outdated Show resolved Hide resolved
hazelcast/metrics.py Show resolved Hide resolved
hazelcast/metrics.py Show resolved Hide resolved
hazelcast/metrics.py Show resolved Hide resolved
hazelcast/statistics.py Outdated Show resolved Hide resolved
hazelcast/statistics.py Outdated Show resolved Hide resolved
hazelcast/statistics.py Show resolved Hide resolved
hazelcast/statistics.py Show resolved Hide resolved
tests/unit/metrics/metrics_test.py Show resolved Hide resolved
@srknzl
Copy link
Member

srknzl commented Aug 5, 2021

Before merging, can you test that if metrics can be sent to management center on windows(python client runs on windows)? some of them like number of file descriptors may not work.

hazelcast/metrics.py Outdated Show resolved Hide resolved
@mdumandag
Copy link
Contributor Author

Yes, I have verified this, on Windows, this feature works with a subset of the stats we sent from Linux.

For example, file descriptor count is not sent on Windows, as there is no such thing in Windows, or the load average will not be sent, as it is not implemented there in the Python standard library.

@codecov-commenter
Copy link

Codecov Report

Merging #441 (7ea0d8c) into master (7d796f2) will decrease coverage by 0.66%.
The diff coverage is 96.37%.

❗ Current head 7ea0d8c differs from pull request most recent head ddfc21d. Consider uploading reports for the commit ddfc21d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
- Coverage   86.62%   85.95%   -0.67%     
==========================================
  Files         336      340       +4     
  Lines       17398    17850     +452     
==========================================
+ Hits        15071    15343     +272     
- Misses       2327     2507     +180     
Impacted Files Coverage Δ
hazelcast/statistics.py 41.80% <91.83%> (-26.98%) ⬇️
hazelcast/metrics.py 82.62% <98.87%> (ø)
hazelcast/reactor.py 77.51% <0.00%> (-6.70%) ⬇️
hazelcast/listener.py 68.60% <0.00%> (-1.17%) ⬇️
hazelcast/connection.py 82.73% <0.00%> (-0.19%) ⬇️
hazelcast/proxy/map.py 73.37% <0.00%> (-0.01%) ⬇️
hazelcast/aggregator.py 97.39% <0.00%> (ø)
hazelcast/protocol/codec/map_aggregate_codec.py 100.00% <0.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d796f2...ddfc21d. Read the comment docs.

@mdumandag mdumandag merged commit c3823a8 into hazelcast:master Aug 6, 2021
@mdumandag mdumandag deleted the binary-metrics branch August 6, 2021 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants