Fix: Use dartLogger as default in debug mode #413

ueman · 2021-04-13T06:49:32Z

📜 Description

In debug mode Sentry now uses the dartLogger (which writes to console) as a logger.
The first thing the SDK does is to create an instance if SentryOptions, so the logical place to put this logic is SentryOptions.
This change also works for Sentry and Sentry Flutter.

💡 Motivation and Context

#384

💚 How did you test it?

I've written new tests.

📝 Checklist

🔮 Next steps

dart/test/hub_test.dart

marandaneto · 2021-04-13T07:40:07Z

PR looks good but it's failing due to pod install, probably not related at all

dart/lib/src/sentry_options.dart

bruno-garcia · 2021-04-13T14:59:58Z

dart/lib/src/sentry.dart

+    // user visible between Sentry's initialization and the invocation of the
+    // options configuration callback.
+    if (options.debug == false && options.logger == dartLogger) {
+      options.logger = noOpLogger;


What about doing this on the setter of logger and of debug?

https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry/SentryOptions.cs#L354-L369

Can we call it diagnosticLogger? If we're bumping a major ideally.

We have some docs on it for .NET for example:
https://docs.sentry.io/platforms/dotnet/configuration/diagnostic-logger/

The goal is to "relate" to the level:
https://docs.sentry.io/platforms/flutter/configuration/options/#diagnostic-level

And to disambiguate with any sort of logging integration we have.

Dart already has a DiagnosticLogger, when you set a logger it already wraps up to a DiagnosticLogger that takes into consideration the diagnosticLevel field.

I think what Bruno meant is to rename the property to diagnosticLogger in order to make its purpose more clear.

The logger is also just a function in the Dart/Flutter SDK

we cant rename now, this would require a major bump, every call to options.logger(params) would change.
what @bruno-garcia meant is, instead of doing such if condition to set a dartLogger or noOpLogger could be encapsulated within a setter.

so options.debug = true (means setting dartLogger)
options.debug = false (means setting noOpLogger).

I like that.

the same could happen if one sets options.logger = function(), this would automatically set options.debug = true and options.logger = null would set options.debug = false, so such conditions are not across the SDK but only setting it enabled or not.

what @bruno-garcia meant is, instead of doing such if condition to set a dartLogger or noOpLogger could be encapsulated within a setter.

Yes! ;)

the same could happen if one sets options.logger = function(), this would automatically set options.debug = true and options.logger = null would set options.debug = false

I wouldn't go this far though. One might change the logger type in an integration but not really be turning debug mode on or off.

I think what Bruno meant is to rename the property to diagnosticLogger in order to make its purpose more clear.

Also this. What about we do this on a next major release? If so, is there a Discussion that keeps tracks of stuff we want to add to the next major?

so options.debug = true (means setting dartLogger)
options.debug = false (means setting noOpLogger).

I'm not sure I would override the logger instance when someone sets false since next time you set it to true you no longer know what instance was set by the user before.

In .NET it's nullable, so that the call to the logger doesn't happen at all and hence no string concat or anything happens in order to invoke the logger. With NoOp you're calling things so any overhead to prepare the log entry is paid even though the logger doesn't do anything. That's just an optimization though, not saying it's something that we need to align. For really expensive operations we can always check: logger.isEnabled(level) before actually calling the logger.

If so, is there a Discussion that keeps tracks of stuff we want to add to the next major?

Now there is: #419

With NoOp you're calling things so any overhead to prepare the log entry is paid even though the logger doesn't do anything. That's just an optimization though, not saying it's something that we need to align. For really expensive operations we can always check: logger.isEnabled(level) before actually calling the logger.

I feel like a callback for the logger would be more ergonomic than a null check.
For example:

logger.log(level, () => 'this callback gets invoked from the logger')

Than we would not need to have the actual code riddled with ifs and what not and also the logging is only computed when needen.

There's an allocation for the callback though. So between the callback and NoOp I'd surely go with the latter.

codecov-commenter · 2021-04-19T06:06:40Z

Codecov Report

Merging #413 (086708d) into main (bb04c3d) will increase coverage by 3.50%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #413      +/-   ##
==========================================
+ Coverage   90.02%   93.52%   +3.50%     
==========================================
  Files          54        5      -49     
  Lines        1684      170    -1514     
==========================================
- Hits         1516      159    -1357     
+ Misses        168       11     -157

Impacted Files	Coverage Δ
dart/lib/src/protocol/sentry_package.dart
dart/lib/src/protocol/dsn.dart
dart/lib/src/sentry_exception_factory.dart
dart/lib/src/transport/noop_transport.dart
dart/lib/src/transport/http_transport.dart
dart/lib/src/protocol/sdk_info.dart
dart/lib/src/platform_checker.dart
dart/lib/src/protocol/sentry_stack_trace.dart
dart/lib/src/noop_client.dart
dart/lib/src/protocol/sdk_version.dart
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb04c3d...086708d. Read the comment docs.

ueman · 2021-04-20T05:40:29Z

So it's not clear to me how I should proceed here. @marandaneto @bruno-garcia

marandaneto · 2021-04-20T07:28:18Z

So it's not clear to me how I should proceed here. @marandaneto @bruno-garcia

let's discuss today in our call

…bug-mode # Conflicts: # dart/lib/src/sentry_options.dart

bruno-garcia · 2021-04-21T00:52:23Z

So it's not clear to me how I should proceed here. @marandaneto @bruno-garcia

let's discuss today in our call

Sorry I missed the call. Let me know if my input is needed, otherwise

…bug-mode

ueman · 2021-04-26T07:43:37Z

Should I set debug = true if the user sets a custom logger?

dart/lib/src/sentry_options.dart

dart/lib/src/sentry.dart

marandaneto

LGTM

ueman added 2 commits April 13, 2021 08:44

Fix: Use dartLogger as default in debug mode

92bb34e

Add PR ID

0e756f0

ueman commented Apr 13, 2021

View reviewed changes

dart/test/hub_test.dart Show resolved Hide resolved

Merge branch 'main' into fix/console-logger-debug-mode

09a8821

ueman marked this pull request as ready for review April 13, 2021 07:48

ueman requested review from bruno-garcia and marandaneto as code owners April 13, 2021 07:48

marandaneto reviewed Apr 13, 2021

View reviewed changes

dart/lib/src/sentry_options.dart Outdated Show resolved Hide resolved

marandaneto reviewed Apr 13, 2021

View reviewed changes

dart/lib/src/sentry_options.dart Outdated Show resolved Hide resolved

Reset dart logger after options configuration callback

5297cd3

bruno-garcia reviewed Apr 13, 2021

View reviewed changes

Set debug = true in sentry options

8820e84

Merge remote-tracking branch 'origin/main' into fix/console-logger-de…

83a09ff

…bug-mode # Conflicts: # dart/lib/src/sentry_options.dart

Merge remote-tracking branch 'origin/main' into fix/console-logger-de…

2c1701b

…bug-mode

ueman mentioned this pull request Apr 26, 2021

Fix: Improve Sentry Integration tests #421

Merged

5 tasks

Set logger in debug setter

c842991

fix analyzer

9075efb

marandaneto reviewed Apr 26, 2021

View reviewed changes

dart/lib/src/sentry_options.dart Show resolved Hide resolved

marandaneto reviewed Apr 26, 2021

View reviewed changes

dart/lib/src/sentry_options.dart Show resolved Hide resolved

marandaneto reviewed Apr 26, 2021

View reviewed changes

dart/lib/src/sentry.dart Outdated Show resolved Hide resolved

Improve code

086708d

marandaneto approved these changes Apr 26, 2021

View reviewed changes

ueman merged commit a1f7888 into main Apr 26, 2021

ueman deleted the fix/console-logger-debug-mode branch April 26, 2021 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Use dartLogger as default in debug mode #413

Fix: Use dartLogger as default in debug mode #413

ueman commented Apr 13, 2021 •

edited

Loading

marandaneto commented Apr 13, 2021

bruno-garcia Apr 13, 2021

marandaneto Apr 13, 2021

ueman Apr 13, 2021 •

edited

Loading

marandaneto Apr 13, 2021

bruno-garcia Apr 14, 2021

ueman Apr 19, 2021

ueman Apr 19, 2021

bruno-garcia Apr 21, 2021 •

edited

Loading

codecov-commenter commented Apr 19, 2021 •

edited

Loading

ueman commented Apr 20, 2021

marandaneto commented Apr 20, 2021

bruno-garcia commented Apr 21, 2021

ueman commented Apr 26, 2021

marandaneto left a comment

Fix: Use dartLogger as default in debug mode #413

Fix: Use dartLogger as default in debug mode #413

Conversation

ueman commented Apr 13, 2021 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

marandaneto commented Apr 13, 2021

bruno-garcia Apr 13, 2021

Choose a reason for hiding this comment

marandaneto Apr 13, 2021

Choose a reason for hiding this comment

ueman Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

marandaneto Apr 13, 2021

Choose a reason for hiding this comment

bruno-garcia Apr 14, 2021

Choose a reason for hiding this comment

ueman Apr 19, 2021

Choose a reason for hiding this comment

ueman Apr 19, 2021

Choose a reason for hiding this comment

bruno-garcia Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Apr 19, 2021 • edited Loading

Codecov Report

ueman commented Apr 20, 2021

marandaneto commented Apr 20, 2021

bruno-garcia commented Apr 21, 2021

ueman commented Apr 26, 2021

marandaneto left a comment

Choose a reason for hiding this comment

ueman commented Apr 13, 2021 •

edited

Loading

ueman Apr 13, 2021 •

edited

Loading

bruno-garcia Apr 21, 2021 •

edited

Loading

codecov-commenter commented Apr 19, 2021 •

edited

Loading