-
Notifications
You must be signed in to change notification settings - Fork 47
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
FEAT: Add Operation throughput and latency metrics by mbean. #841
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1차 리뷰 의견입니다.
src/main/java/net/spy/memcached/metrics/OpThroughputMonitor.java
Outdated
Show resolved
Hide resolved
src/main/java/net/spy/memcached/protocol/BaseOperationImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/net/spy/memcached/metrics/OpThroughputMonitor.java
Outdated
Show resolved
Hide resolved
src/main/java/net/spy/memcached/metrics/OpThroughputMonitor.java
Outdated
Show resolved
Hide resolved
src/main/java/net/spy/memcached/metrics/OpThroughputMonitor.java
Outdated
Show resolved
Hide resolved
31929f7
to
10c2595
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2차 리뷰 의견입니다.
src/main/java/net/spy/memcached/metrics/LatencyMetricsSnapShot.java
Outdated
Show resolved
Hide resolved
10c2595
to
de52fc7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이것만 수정하면 될 것 같습니다.
src/main/java/net/spy/memcached/metrics/LatencyMetricsSnapShot.java
Outdated
Show resolved
Hide resolved
de52fc7
to
00163e5
Compare
이 PR을 리뷰하기전에 본 구현이 어떤 목적의 변경인지 또는 해당 구현에 대한 설명이 필요하신가요? |
@brido4125 |
long elapsedSeconds = (long) ((currentTime - lastTime) / 1000.0); | ||
|
||
return countValue / elapsedSeconds; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아래 내용 중에 잘못 이해한 부분이 있나요?
resetStatistics()
직접 호출하지 않으면lastTime
은 변하지 않는 값입니다.- 따라서
elapsedSeconds
는 계속해서 커지게 됩니다. - 구동 이후 충분한 시간이 흐른 뒤에는, 순간적인 요청량 변화가 Throughput에 드러나지 않게 됩니다.
예를 들어,
모니터링 시작 이후 하루(86,400 초)동안 요청이 없으면, CompleteOps metric 값은 0입니다.
하루가 지나는 시점에 초당 10,000개 요청을 1분간 처리한 후 CompleteOps metric 다시 확인해 보면,
초당 처리량은 6 (600,000 / 86,460 = 6.93…
)으로 나타날 것 같은데, 맞나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node-exporter나 arcus-exporter 등의 다른 메트릭 수집프로그램에서는
초당 요청량 등을 보통 어떻게 처리하나요?
알고 있는 context가 있으면 설명 부탁드려요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
metric type은 대부분 counter와 gauge로 나눌 수 있는데, 둘의 차이를 알고 있나요?
요청량과 같은 수치는 일반적으로 exporter 수준에서 초당 변화량을 계산하지 않고, 누적값을 counter metric으로 내보냅니다.
예를 들어, arcus_cmd_get metric은 초당 get 요청 처리량
이 아니라
구동 이후 누적 get 요청 처리량
(== memcached stats 결과를 그대로 사용)이고, 계속해서 증가만 하는 값입니다.
실제로 초당 요청량은 Grafana에 작성된 promQL 질의가 수행되면서 계산됩니다.
https://prometheus.io/docs/prometheus/latest/querying/examples/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throughput 관련 메트릭들은 counter 타입으로 수집되도록 변경했습니다.
00163e5
to
314e131
Compare
🔗 Related Issue
https://github.com/jam2in/arcus-works/issues/615
⌨️ What I did
아래의 정보를 MBean을 통해 노출시켜 jmx-export를 통해 수집될 수 있도록 하였습니다.
메트릭들의 수집 단위는 하나의 java process 입니다.
즉, N개의 ArcusClient 전부에 대한 수집방식입니다.
그라파나 대시보드는 이슈 내에 기입되어 있습니다.