-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add memory & conn collector, collector manager, tests #460
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #460 +/- ##
===========================================
- Coverage 90.72% 90.65% -0.08%
===========================================
Files 60 65 +5
Lines 3818 4494 +676
===========================================
+ Hits 3464 4074 +610
- Misses 354 420 +66
|
b6198a4
to
85a303a
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.
Okay! Still making my way through this PR, but so far looks very impressive. I commented on a lot of the "low hanging fruit" so you at least have something to go off of while you're sorting out why the CI is failing!
I promise to have a more complete review your way ASAP!!
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.
Couple minor questions, overall looks very good
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.
Couple of other last minute, pedantic things I noticed while reviewing the tests!
Sets the default value of the "enable telemetry" flag to on. Currently this will enable telemetry system wide until finer grain control can be established with CrayLabs#460
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.
Nice work! I like the overall structure. I have some comments/feedback that are geared towards future-proofing complexity and scale.
This PR makes several patch changes to prepare for a SmartSim release including: - Set the default value of the "enable telemetry" flag to on. Currently this will enable telemetry system wide until finer grain control can be established with #460 - Bump the output `manifest.json` version number to match that of `smartdashboard` - Pin a watchdog version to avoid build errors [ committed by @MattToast @ankona ] [ reviewed by @ankona ] --------- Co-authored-by: Christopher McBride <christopher.mcbride@gmail.com>
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.
Two quick comments
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.
I have some truly menial docstr/typing comments, but otherwise approval still stands!
New features
CollectorManager
. Metrics are written using the newFileSink
classIncluded collectors:
DbMemoryCollector
DbConnectionCollector
Updated features
smartsim._core.utils.telemetry
.