-
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
[#1325] Log metrics #1327
[#1325] Log metrics #1327
Conversation
b9b9f1d
to
b7039fc
Compare
- the logger has been extended to log the level as well - there is only partial match of levels between SDK logger levels, OSLogEntryLogLevel and OSLogType so only debug, info, error are fully matched - this is a base for the exporter on client's side [Electric-Coin-Company#1325] Log metrics - typos fixed [Electric-Coin-Company#1325] Log metrics - scan metric logs added [Electric-Coin-Company#1325] Log metrics - Scan & Enhance logs [Electric-Coin-Company#1325] Log metrics - checkpoints updated - every CBP action is measured separately and collects the data, when the sync is done it dumps overview of the run to the logger - next run clears out the previous data and starts to collect fresh reports for the run [Electric-Coin-Company#1325] Log metrics (Electric-Coin-Company#1327) - changelog update
- the logger has been extended to log the level as well - there is only partial match of levels between SDK logger levels, OSLogEntryLogLevel and OSLogType so only debug, info, error are fully matched - this is a base for the exporter on client's side [Electric-Coin-Company#1325] Log metrics - typos fixed [Electric-Coin-Company#1325] Log metrics - scan metric logs added [Electric-Coin-Company#1325] Log metrics - Scan & Enhance logs [Electric-Coin-Company#1325] Log metrics - checkpoints updated - every CBP action is measured separately and collects the data, when the sync is done it dumps overview of the run to the logger - next run clears out the previous data and starts to collect fresh reports for the run [Electric-Coin-Company#1325] Log metrics (Electric-Coin-Company#1327) - changelog update [Electric-Coin-Company#1325] Log metrics (Electric-Coin-Company#1327) - SDKMetrics updated to be mockable - unit test updated
b7039fc
to
1c2c9a5
Compare
- the logger has been extended to log the level as well - there is only partial match of levels between SDK logger levels, OSLogEntryLogLevel and OSLogType so only debug, info, error are fully matched - this is a base for the exporter on client's side [Electric-Coin-Company#1325] Log metrics - typos fixed [Electric-Coin-Company#1325] Log metrics - scan metric logs added [Electric-Coin-Company#1325] Log metrics - Scan & Enhance logs [Electric-Coin-Company#1325] Log metrics - checkpoints updated - every CBP action is measured separately and collects the data, when the sync is done it dumps overview of the run to the logger - next run clears out the previous data and starts to collect fresh reports for the run [Electric-Coin-Company#1325] Log metrics (Electric-Coin-Company#1327) - changelog update [Electric-Coin-Company#1325] Log metrics (Electric-Coin-Company#1327) - SDKMetrics updated to be mockable - unit test updated [Electric-Coin-Company#1325] Log metrics (Electric-Coin-Company#1327) - performance tests cleaned out
1c2c9a5
to
18aac73
Compare
- the logger has been extended to log the level as well - there is only partial match of levels between SDK logger levels, OSLogEntryLogLevel and OSLogType so only debug, info, error are fully matched - this is a base for the exporter on client's side [Electric-Coin-Company#1325] Log metrics - typos fixed [Electric-Coin-Company#1325] Log metrics - scan metric logs added [Electric-Coin-Company#1325] Log metrics - Scan & Enhance logs [Electric-Coin-Company#1325] Log metrics - checkpoints updated - every CBP action is measured separately and collects the data, when the sync is done it dumps overview of the run to the logger - next run clears out the previous data and starts to collect fresh reports for the run [Electric-Coin-Company#1325] Log metrics (Electric-Coin-Company#1327) - changelog update [Electric-Coin-Company#1325] Log metrics (Electric-Coin-Company#1327) - SDKMetrics updated to be mockable - unit test updated [Electric-Coin-Company#1325] Log metrics (Electric-Coin-Company#1327) - performance tests cleaned out [Electric-Coin-Company#1325] Log metrics (Electric-Coin-Company#1327) - Network tests buildable again
18aac73
to
3e263f0
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.
Look's good to me. I left only a few notes inline. Plus, here are general questions:
- What is the motivation for removing enable/disable API from the metrics collecting logic? We could still have switching logging and collecting metrics in the public API.
- Could we prepare tests on ensuring expected logs are pasted into the device logger stack and another for SDKMetricks?
Mainnet | ||
|
||
```` | ||
Sources/ZcashLightClientKit/Resources/checkpoints/mainnet/2270000.json |
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.
Should we inform about checkpoints in the changelog? It's an SDK internal implementation, which we could mention, but we don't necessarily need to share details about it.
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.
Pacu started this initiative in this structure so I continue. We may not need to mention it but SDK changelog is a bit more technical than Zashi one. So I didn't find it pointless.
import XCTest | ||
@testable import ZcashLightClientKit | ||
|
||
final class SDKMetricsTests: XCTestCase { |
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.
Should we rework these tests to have the SDKMetrics logic still covered by tests?
Closes #1325
This code review checklist is intended to serve as a starting point for the author and reviewer, although it may not be appropriate for all types of changes (e.g. fixing a spelling typo in documentation). For more in-depth discussion of how we think about code review, please see Code Review Guidelines.
Author
Reviewer