-
Notifications
You must be signed in to change notification settings - Fork 3
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 metrics base #84
Conversation
ef9e811
to
3057f7c
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.
This is a really good start. Are the metrics currently accessible on port 9999? Should we augment the README.md to let devs know how to see the data.
server/src/main/java/com/hedera/block/server/config/BlockNodeConfigExtension.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/config/BlockNodeConfigExtension.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/config/BlockNodeContext.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/config/BlockNodeContextFactory.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/config/BlockNodeContextFactory.java
Show resolved
Hide resolved
server/src/test/java/com/hedera/block/server/config/BlockNodeContextFactoryTest.java
Show resolved
Hide resolved
server/src/test/java/com/hedera/block/server/metrics/MetricsServiceTest.java
Outdated
Show resolved
Hide resolved
8273015
to
4c6ccd2
Compare
I added a metrics.md under docs with general information about the metrics implementation and usage. |
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.
LGTM
…form sdk needs Signed-off-by: Alfredo Gutierrez <[email protected]>
…tFactory as well as MetricsService. All of them are necessary to be able to create MetricsService and add Metrics to the project, but will also be necessary when importing other modules from hedera platform-sdk Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
changed logger from log4j to system.loggger Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
e307132
to
5ca72a8
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.
Nothing blocking, but some concerns worth addressing in a follow-up.
A question about the autoservice
dependencies.
A suggestion to upgrade from simpleclient
to the replacement java_client
for prometheus.
I also have quite a few suggestions for Javadoc (though I stopped adding them after the first several). What's here is very typical "checkbox" Javadoc and adds very little real value. While that is not "bad" per se, it's a far distance from what I would consider ideal.
In my opinion we should strive, as much as resources and time permit, to produce well written, complete, well structured, and thorough Javadoc specification for all public and package-private methods and classes (at minimum).
I would be happy to walk through a class or two with you if that would help to align on what a high quality Javadoc API specification looks like.
server/src/main/java/com/hedera/block/server/config/BlockNodeContext.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/config/BlockNodeConfigExtension.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/config/BlockNodeContext.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/config/BlockNodeContextFactory.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/config/BlockNodeConfigExtension.java
Show resolved
Hide resolved
Thank you @jsync-swirlds for your valuable review. I have answered questions about the dependencies in question and indeed are requiered by Platform SDK. Did the suggestion for a specific comment and I accept your offer of a walk through on java docs so I can improve my javadoc skills and quality. :) |
Signed-off-by: Alfredo Gutierrez <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #84 +/- ##
============================================
+ Coverage 64.82% 70.00% +5.17%
- Complexity 25 33 +8
============================================
Files 6 10 +4
Lines 145 170 +25
Branches 6 6
============================================
+ Hits 94 119 +25
Misses 47 47
Partials 4 4
|
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.
Looks Good
Description:
In order to be able to add metrics base from platform-sdk the following dependencies were added:
Also including a MetricsService that will contain the metrics of the BlockNode Server, so their definitions are centralized and the same metric can be used in different places if needed.
Verifying that metrics are exposed on
<server>:9999/metrics
Related issue(s):
Fixes #66
Fixes #69
Fixes #68
Notes for reviewer:
Note new dependencies brought by added packages:
/gradlew :server:dependencies
Checklist