Add integration for logging package #631
Conversation
Codecov Report
@@ Coverage Diff @@
## main #631 +/- ##
==========================================
+ Coverage 90.28% 90.38% +0.09%
==========================================
Files 93 95 +2
Lines 3038 3088 +50
==========================================
+ Hits 2743 2791 +48
- Misses 295 297 +2
Continue to review full report at Codecov.
|
|
@ueman thanks a lot for doing this, will take a look at it next week :) |
|
@marandenato Did you have time yet? |
not yet, sorry about that, we usually have the meeting about new features on Wed. but we've split up and it got postponed due to schedule conflicts, will have a look latest the end of the week |
|
thanks a lot for doing this @ueman , that's great. |
Co-authored-by: Manoel Aranda Neto <[email protected]>
Co-authored-by: Manoel Aranda Neto <[email protected]>
Co-authored-by: Manoel Aranda Neto <[email protected]>
|
missing changelog, and almost there :) |
|
It has a changelog, just not in the Changelog of the Dart/Flutter SDK. Danger probably has to be changed to account for the new logging package. |
|
@ueman lets rename the |
|
Okay, will do |
marandaneto
left a comment
marandaneto
left a comment
There was a problem hiding this comment.
LGTM, thanks for doing this @ueman
let's see if @denrase or @brustolin has any insight, otherwise good to go.
denrase
left a comment
denrase
left a comment
There was a problem hiding this comment.
Nice! We could add tests for the case where the logged level is equal to the min level and by this define the behaviour of it. Also maybe consider using mockito instead of the mock class if that makes sense at all here.
|
|
||
| final fakeDsn = 'https://[email protected]/1234567'; | ||
|
|
||
| class MockHub implements Hub { |
There was a problem hiding this comment.
Since we already use mockito, would it also be feasible to use it here instead of adding our own mock implementation?
There was a problem hiding this comment.
I recall us running away from mockito because of a few issues (eg null safety back then), right now we probably have a mix of mockito + fakes, right? ideally, we'd stick to one of them going forward, I'd rather go with the fakes to avoid problems as in the past.
There was a problem hiding this comment.
Yeah i just ran into issues with mockito and and nullability, this can be resolved using fallback generators. Used this in #643, but is is not ideal.
| @override | ||
| FutureOr<void> call(Hub hub, SentryOptions options) { | ||
| _hub = hub; | ||
| _setSdkVersion(options); |
There was a problem hiding this comment.
@ueman @marandaneto Should the SDK version be set here? This reports sentry.dart.logging .
There was a problem hiding this comment.
Does it also report sentry.dart.logging for exceptions captured by sentry_flutter or sentry?
In general, every exception captured by sentry_logging should have sentry.dart.logging as its origin.
There was a problem hiding this comment.
From what I understand, this is called only once when the integration is registered and overrides the original SDK.
There was a problem hiding this comment.
Looks like this should not be set https://github.com/getsentry/sentry-dart/blob/main/logging/lib/src/logging_integration.dart#L49-L50
Since its an Integration rather than a top-level package, adding to the package list https://github.com/getsentry/sentry-dart/blob/main/logging/lib/src/logging_integration.dart#L54
Adding to the integration list https://github.com/getsentry/sentry-dart/blob/main/logging/lib/src/logging_integration.dart#L38 and setting the logger https://github.com/getsentry/sentry-dart/blob/main/logging/lib/src/extension.dart#L27 should have been enough.
Would you like to submit a PR @kuhnroyal ?
6 participants
📜 Description
This is an integration which adds support for the logging package. It's the official dart logging solution.
I've added it as a seperate package in order to not bloat the Sentry Dart package.
💡 Motivation and Context
Closes #555
💚 How did you test it?
New tests and CI
📝 Checklist
🔮 Next steps
Update docs.sentry.io