Skip to content
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

refactor: use options.clock wherever possible #1110

Merged
merged 6 commits into from
Nov 10, 2022
Merged

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Nov 7, 2022

📜 Description

Closes #811

#skip-changelog

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

CHANGELOG.md Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2022

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 299.02 ms 347.63 ms 48.61 ms
Size 5.94 MiB 6.95 MiB 1.01 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
559d28f 302.35 ms 339.53 ms 37.18 ms
322aa66 284.98 ms 341.76 ms 56.78 ms
7ade5af 341.04 ms 386.84 ms 45.80 ms
9c5aec6 287.33 ms 320.94 ms 33.61 ms
56810ff 309.72 ms 352.26 ms 42.54 ms
0ceb89c 304.57 ms 357.18 ms 52.61 ms
04db237 330.16 ms 428.38 ms 98.22 ms
72dfc83 298.62 ms 340.14 ms 41.52 ms
6d317ea 303.46 ms 356.06 ms 52.60 ms
d7758e8 300.12 ms 349.88 ms 49.76 ms

App size

Revision Plain With Sentry Diff
559d28f 5.94 MiB 6.92 MiB 1001.70 KiB
322aa66 5.94 MiB 6.92 MiB 1005.75 KiB
7ade5af 5.94 MiB 6.95 MiB 1.01 MiB
9c5aec6 5.94 MiB 6.92 MiB 1001.61 KiB
56810ff 5.94 MiB 6.92 MiB 1001.71 KiB
0ceb89c 5.94 MiB 6.95 MiB 1.01 MiB
04db237 5.94 MiB 6.95 MiB 1.01 MiB
72dfc83 5.94 MiB 6.92 MiB 1001.71 KiB
6d317ea 5.94 MiB 6.92 MiB 1001.74 KiB
d7758e8 5.94 MiB 6.95 MiB 1.01 MiB

Previous results on branch: fix/use-options-clock

Startup times

Revision Plain With Sentry Diff
27b6b05 326.92 ms 373.36 ms 46.44 ms
69c6b69 334.23 ms 433.43 ms 99.20 ms

App size

Revision Plain With Sentry Diff
27b6b05 5.94 MiB 6.95 MiB 1.01 MiB
69c6b69 5.94 MiB 6.95 MiB 1.01 MiB

@vaind vaind changed the title fix: use options.clock as endtimestamp fix: use options.clock wherever possible Nov 7, 2022
@vaind
Copy link
Collaborator Author

vaind commented Nov 7, 2022

Outstanding - dartLogger() - should we also use options.clock there, somehow?

/// A Logger that prints out the level and message
void dartLogger(
  SentryLevel level,
  String message, {
  String? logger,
  Object? exception,
  StackTrace? stackTrace,
}) {
  log(
    '[${level.name}] $message',
    level: level.toDartLogLevel(),
    name: logger ?? 'sentry',
    time: getUtcDateTime(),
    error: exception,
    stackTrace: stackTrace,
  );
}

@vaind vaind marked this pull request as ready for review November 7, 2022 17:03
@marandaneto
Copy link
Contributor

@vaind lets add @internal to SentryOptions#clock since it should not be used by people.

@vaind
Copy link
Collaborator Author

vaind commented Nov 8, 2022

@vaind lets add @internal to SentryOptions#clock since it should not be used by people.

OK, with the final comment about making the option internal, I agree it is fine to remove toUTC(). BTW when you do a review as a whole (as opposed to individual comments), the final comment would show above code-review comments and would better clarify the intent.

@marandaneto
Copy link
Contributor

If possible, I'd say so, the reason to use options.clock everywhere is testability and reusability.
Plus single source of truth for date and time.

@vaind
Copy link
Collaborator Author

vaind commented Nov 9, 2022

Implemented the requested changes, including the default debug logger change. I've kept public dartLogger untouched - not sure it should really be public but there's nothing for it now as it may be used by client code. At best, it could be made deprecated.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1255.67 ms 1269.73 ms 14.06 ms
Size 8.16 MiB 9.15 MiB 1021.23 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
f922f8f 1249.53 ms 1266.51 ms 16.98 ms
04db237 1273.29 ms 1306.50 ms 33.21 ms
3a69405 1292.84 ms 1303.96 ms 11.12 ms
72dfc83 1262.50 ms 1289.75 ms 27.25 ms
56810ff 1267.59 ms 1293.48 ms 25.89 ms
559d28f 1265.04 ms 1288.96 ms 23.92 ms
9c5aec6 1266.51 ms 1274.65 ms 8.14 ms
eb1a7c1 1281.25 ms 1295.40 ms 14.15 ms
4efee31 1270.33 ms 1285.75 ms 15.42 ms
6957bfd 1283.80 ms 1289.00 ms 5.20 ms

App size

Revision Plain With Sentry Diff
f922f8f 8.15 MiB 9.13 MiB 1003.20 KiB
04db237 8.15 MiB 9.13 MiB 1003.16 KiB
3a69405 8.15 MiB 9.15 MiB 1018.56 KiB
72dfc83 8.15 MiB 9.12 MiB 987.30 KiB
56810ff 8.15 MiB 9.12 MiB 987.35 KiB
559d28f 8.15 MiB 9.12 MiB 987.32 KiB
9c5aec6 8.15 MiB 9.12 MiB 986.23 KiB
eb1a7c1 8.15 MiB 9.13 MiB 1000.10 KiB
4efee31 8.15 MiB 9.12 MiB 991.35 KiB
6957bfd 8.15 MiB 9.15 MiB 1020.71 KiB

Previous results on branch: fix/use-options-clock

Startup times

Revision Plain With Sentry Diff
27b6b05 1279.54 ms 1295.98 ms 16.44 ms

App size

Revision Plain With Sentry Diff
27b6b05 8.15 MiB 9.15 MiB 1021.18 KiB

@marandaneto
Copy link
Contributor

Implemented the requested changes, including the default debug logger change. I've kept public dartLogger untouched - not sure it should really be public but there's nothing for it now as it may be used by client code. At best, it could be made deprecated.

Let's add @deprecated then, so we remove in the next bump.

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a last comment, besides that, LGTM

@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2022

Codecov Report

Base: 91.66% // Head: 91.66% // No change to project coverage 👍

Coverage data is based on head (2f2a9a9) compared to base (2f2a9a9).
Patch has no changes to coverable lines.

❗ Current head 2f2a9a9 differs from pull request most recent head 3b7603d. Consider uploading reports for the commit 3b7603d to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1110   +/-   ##
=======================================
  Coverage   91.66%   91.66%           
=======================================
  Files           9        9           
  Lines         192      192           
=======================================
  Hits          176      176           
  Misses         16       16           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@vaind vaind changed the title fix: use options.clock wherever possible refactor: use options.clock wherever possible Nov 10, 2022
@vaind vaind enabled auto-merge (squash) November 10, 2022 12:00
@vaind vaind merged commit 25e9b59 into main Nov 10, 2022
@vaind vaind deleted the fix/use-options-clock branch November 10, 2022 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use SentryOptions#clock whenever possible instead of getUtcDateTime or DateTime.now() directly.
3 participants