-
Notifications
You must be signed in to change notification settings - Fork 6
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
chore: Core crypto 2.0 migration [WPB-14635] #3141
Conversation
@@ -36,12 +39,19 @@ actual suspend fun coreCryptoCentral( | |||
key = databaseKey | |||
) | |||
coreCrypto.setCallbacks(Callbacks()) | |||
setLogger(CoreCryptoLoggerImpl(), CoreCryptoLogLevel.INFO) |
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.
would we want to have some other log level for debug builds?
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.
Maybe we can set to the lowest level (CoreCryptoLogLevel.Trace
), and within the CoreCryptoLoggerImpl
we can map to different kaliumLogger
functions, which alreadys implement log-level filtering within 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.
Warning
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.
@typfel should we then make it default to INFO
? or we should have it disabled for prod at all?
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.
We want it enabled in Prod at the INFO
level which will not generate large amounts of logs.
Setting the level TRACE
will potentially generate a lot of logs so it should not be enabled in Prod.
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.
Reverted to INFO
there
} | ||
|
||
@Suppress("MagicNumber") | ||
private fun mapProteusExceptionToRawIntErrorCode(proteusException: ProteusExceptionNative): Int { |
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.
Those mappers are here because we've to rely on the native ProteusException
type directly, and we're not having the right dependency in the ProteusException
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.
Going forward I'd suggest aligning ProteusException
with the ProteusExceptionNative
. We want to go away from dealing Int
numbers for error handling.
Doesn't have to be done right away but maybe create a TODO with a ticket?
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.
Makes sense to have it in a different task, there is also some corecryptobox mappings to the ProteusException
- but indeed would be nice to also clean it up
Bencher Report
Click to view all benchmark results
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3141 +/- ##
===========================================
+ Coverage 54.18% 54.19% +0.01%
===========================================
Files 1251 1251
Lines 36403 36444 +41
Branches 3680 3689 +9
===========================================
+ Hits 19725 19752 +27
- Misses 15259 15268 +9
- Partials 1419 1424 +5
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportBranch report: ✅ 0 Failed, 3232 Passed, 107 Skipped, 1m 8.07s Total Time |
@@ -36,12 +39,19 @@ actual suspend fun coreCryptoCentral( | |||
key = databaseKey | |||
) | |||
coreCrypto.setCallbacks(Callbacks()) | |||
setLogger(CoreCryptoLoggerImpl(), CoreCryptoLogLevel.INFO) |
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.
Maybe we can set to the lowest level (CoreCryptoLogLevel.Trace
), and within the CoreCryptoLoggerImpl
we can map to different kaliumLogger
functions, which alreadys implement log-level filtering within it.
cryptography/src/commonJvmAndroid/kotlin/com.wire.kalium.cryptography/CoreCryptoCentral.kt
Outdated
Show resolved
Hide resolved
Quality Gate passedIssues Measures |
private class CoreCryptoLoggerImpl : CoreCryptoLogger { | ||
override fun log(level: CoreCryptoLogLevel, message: String, context: String?) { | ||
when (level) { | ||
CoreCryptoLogLevel.TRACE -> kaliumLogger.v("$message. $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.
@m-zagorski is the message and the context concatenated together? Or do they get sent to datadog separately?
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.
the message will be concatenated - is that correct ? @johnxnguyen
https://wearezeta.atlassian.net/browse/WPB-14635
What's new in this PR?
Solutions
Update core crypto to version 2.0.0. Migrate exception error codes mappings, add logger with default log level set to
INFO
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764
.