feat: Track timezone changes as breadcrumbs #1930

kevinrenskers · 2022-07-04T12:33:55Z

📜 Description

When the timezone changes, the system sends a notification. We track this event as a breadcrumb.

💡 Motivation and Context

💚 How did you test it?

Unit test. Also ran the sample app, changed my device's timezone, and confirmed that a breadcrumb was posted to Sentry: https://sentry.io/organizations/sentry-sdks/issues/2895893229/?project=5428557#breadcrumbs

📝 Checklist

I reviewed the submitted code
I added tests to verify the changes
I updated the docs if needed
Review from the native team if needed
No breaking changes

🔮 Next steps

github-actions · 2022-07-04T12:34:40Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 2571228

kevinrenskers · 2022-07-04T12:36:44Z

This currently is very very simple: it just sends a breadcrumb with the action kCFTimeZoneSystemTimeZoneDidChangeNotification. Do we want to log the old and the new timezone as well?

Also the issue says "Ideally we persist previous runs breadcrumbs so steps from a previous run can help diagnose issues related to time changing." but I am not sure what is meant by that.

Sentry.xcodeproj/project.pbxproj

philipphofmann · 2022-07-04T13:12:59Z

This currently is very very simple: it just sends a breadcrumb with the action kCFTimeZoneSystemTimeZoneDidChangeNotification. Do we want to log the old and the new timezone as well?

What would be important to you as a developer?

Also the issue says "Ideally we persist previous runs breadcrumbs so steps from a previous run can help diagnose issues related to time changing." but I am not sure what is meant by that.

I added a comment #1763 (comment).

philipphofmann

First glance. Thanks for tackling this, @kevinrenskers.

CHANGELOG.md

Sentry.xcodeproj/project.pbxproj

Sources/Sentry/SentrySystemEventBreadcrumbs.m

* master: release: 7.19.0

* master: build(deps): bump fastlane from 2.206.2 to 2.207.0 (#1937) fixing sample not compiling (#1935) style: Configure the root of the project to use four spaces for indentation (#1934) meta: Add LOGAF scale to Contributing (#1933) meta: Mention draft PRs in Contributing (#1932) meta: Add sut to contributing (#1931)

…ng instantiated when it's missing on disk

Provide a default value to deal with test crashes, until we can properly fix the tests

kevinrenskers · 2022-07-05T09:50:36Z

The feature works perfectly fine in the sample apps. Initially the timezone is nil, so we store it, no breadcrumb gets sent. From then on, every time the app starts it can detect timezone changes, and while the app is alive it'll also react to timezone changes, and sent breadcrumbs.

However, the test is problematic. First of all, because updating the app state doesn't work when there is no app state stored on disk yet. And there are other weird things going on with the file manager and the app state manager. Dhiogo can say more about that I think. I made the test pass consistently by adding a default value in SentrySystemEventBreadcrumbs on line 219, but that really shouldn't be necessary.

Also, this code only runs on iOS because the app state manager doesn't compile for macOS. Ideally this feature should work on macOS.

philipphofmann

Well done, not bad for not using ObjC for 6 years 😁👏 . Let's have a chat about how to test this at our meeting today.

Sources/Sentry/include/SentryAppState.h

philipphofmann · 2022-07-05T10:03:18Z

Sources/Sentry/include/SentryAppState.h

@@ -42,6 +43,8 @@ SENTRY_NO_INIT

 @property (nonatomic, assign) BOOL isANROngoing;

+@property (nonatomic, copy) NSNumber *_Nullable timezoneOffset;


m: We use three different data types. In the constructor, timezoneOffset is NSTimeInterval, here it is NSNumber and [NSTimeZone localTimeZone].secondsFromGMT] is NSInteger. Can we please align these. I would suggest to use NSInteger everywhere and renaming this to secondsFromGMT.

Ah right, the NSTimeInterval was from a different property I used for a while, that's easily changed to NSInteger.

But changing the NSNumber to NSInteger.. can NSInteger be null? I really need to refresh my knowledge of optionals in Obj-C 😅 I made it an NSNumber specifically because of the optionality.

and renaming this to secondsFromGMT

I think timezoneOffset is a lot more clear. secondsFromGMT doesn't really tell you that this about a timezone thing.

Sources/Sentry/SentryAppStateManager.m

Tests/SentryTests/Helper/SentryAppStateTests.swift

Sources/Sentry/SentrySystemEventBreadcrumbs.m

Tests/SentryTests/Integrations/Breadcrumbs/SentrySystemEventBreadcrumbsTest.swift

philipphofmann · 2022-07-05T10:28:05Z

Also, this code only runs on iOS because the app state manager doesn't compile for macOS. Ideally this feature should work on macOS.

I would be fine with this tradeoff for now.

kevinrenskers · 2022-07-05T16:12:23Z

Ok, got further along but testTimezoneChangeBreadcrumb is still problematic. It now always succeeds when I run it in isolation, but it fails when I run the entire SentrySystemEventBreadcrumbsTest class.

The idea is that I want to clear the stored timezone offset (from the filemanager), so that we always start with a clean slate. Then our mocked current date provider will say the timezone offset is 0, we initialize the SentrySystemEventBreadcrumbs (which will write the offset to disk), then we change the offset to another number and post the notification.

That all works in isolation, but not when running all the tests. Any ideas?

kevinrenskers · 2022-07-06T08:41:11Z

Tests are fixed, and more tests are added. I think this is ready for review now!

kevinrenskers · 2022-07-06T09:22:34Z

Getting a random SentryANRTrackerTests failure in the Catalyst on Xcode 12 tests.

philipphofmann · 2022-07-06T09:48:43Z

Getting a random SentryANRTrackerTests failure in the Catalyst on Xcode 12 tests.

Don't worry about these in this PR. It seems they are flaky and we should fix them.

philipphofmann

Great job, just a few missing tests for SentryFileManager.

Sources/Sentry/SentryFileManager.m

Sources/Sentry/SentrySystemEventBreadcrumbs.m

Tests/SentryTests/Integrations/Breadcrumbs/SentrySystemEventBreadcrumbsTest.swift

philipphofmann

LGTM 🚀

kevinrenskers added 2 commits July 4, 2022 13:52

feat: Add automatic timezone change reporting as a breadcrumb

cd2e0c0

Add a test for the new breadcrumb

69c98c5

Format code

b035930

kevinrenskers added 2 commits July 4, 2022 14:39

Add changelog entry

1434409

Indent with 4 spaces for all source files

1ecd302

kevinrenskers commented Jul 4, 2022

View reviewed changes

Sentry.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved

Fix testTimezoneChangeBreadcrumb by using the Swiftified name

543bedb

philipphofmann reviewed Jul 4, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Sentry.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved

kevinrenskers added 2 commits July 4, 2022 15:30

Actually fix the tests

b90c52d

Improve the start method's logic, prevent duplicate call in case of iOS

4112017

brustolin reviewed Jul 4, 2022

View reviewed changes

Sources/Sentry/SentrySystemEventBreadcrumbs.m Outdated Show resolved Hide resolved

kevinrenskers and others added 9 commits July 4, 2022 15:42

Undo indentation change, it'll be in a separate PR

ab971ed

Merge branch 'master' into feat/timezone-change-breadcrumbs

de3547d

* master: release: 7.19.0

Feature complete, but test is flaky

81102cc

Format code

4118523

timezoneOffset is nullable and shouldn't stop SentryAppState from bei…

694ed71

…ng instantiated when it's missing on disk

Only run the code in iOS, because it doesn't compile for macOS

9d5b0ac

Provide a default value to deal with test crashes, until we can properly fix the tests

Format code

82fe669

Deal with timezoneOffset == nil

f453208

kevinrenskers marked this pull request as ready for review July 5, 2022 09:30

philipphofmann reviewed Jul 5, 2022

View reviewed changes

kevinrenskers and others added 3 commits July 5, 2022 13:09

Commit first set of suggestions

8d13cd4

Format code

b83f59c

Add more tests

1a4f01e

kevinrenskers and others added 9 commits July 5, 2022 15:23

It works, but tests need to be fixed

a0b6087

Format code

cb42c7e

Don't cache fileManager, as it can change when the hub changes

6c7b428

Use currentDateProvider

9f9e79a

Format code

c585e80

Undo

f808338

Tests compile again

4f7edd3

Fix warning

97acf8d

testTimezoneChangeBreadcrumb is still problematic

bc9d991

Added more tests, and the whole suit runs successfully

8ce9385

kevinrenskers force-pushed the feat/timezone-change-breadcrumbs branch from d4e51ad to 8ce9385 Compare July 6, 2022 08:37

Format code

70b982e

philipphofmann reviewed Jul 6, 2022

View reviewed changes

kevinrenskers and others added 3 commits July 6, 2022 12:39

Add more filemanager tests

0ad0783

Format code

0996314

One more test

2571228

philipphofmann approved these changes Jul 6, 2022

View reviewed changes

kevinrenskers merged commit 53e3805 into master Jul 6, 2022

kevinrenskers deleted the feat/timezone-change-breadcrumbs branch July 6, 2022 11:12

armcknight mentioned this pull request Aug 23, 2022

Fix SentryTests.SentrySessionTrackerTests Flaky tests #2062

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Track timezone changes as breadcrumbs #1930

feat: Track timezone changes as breadcrumbs #1930

kevinrenskers commented Jul 4, 2022 •

edited by philipphofmann

github-actions bot commented Jul 4, 2022 •

edited

kevinrenskers commented Jul 4, 2022

philipphofmann commented Jul 4, 2022

philipphofmann left a comment

kevinrenskers commented Jul 5, 2022 •

edited

philipphofmann left a comment

philipphofmann Jul 5, 2022

kevinrenskers Jul 5, 2022

kevinrenskers Jul 5, 2022

philipphofmann commented Jul 5, 2022

kevinrenskers commented Jul 5, 2022

kevinrenskers commented Jul 6, 2022

kevinrenskers commented Jul 6, 2022

philipphofmann commented Jul 6, 2022

philipphofmann left a comment

philipphofmann left a comment

		@@ -42,6 +43,8 @@ SENTRY_NO_INIT

		@property (nonatomic, assign) BOOL isANROngoing;

		@property (nonatomic, copy) NSNumber *_Nullable timezoneOffset;

feat: Track timezone changes as breadcrumbs #1930

feat: Track timezone changes as breadcrumbs #1930

Conversation

kevinrenskers commented Jul 4, 2022 • edited by philipphofmann

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Jul 4, 2022 • edited

kevinrenskers commented Jul 4, 2022

philipphofmann commented Jul 4, 2022

philipphofmann left a comment

Choose a reason for hiding this comment

kevinrenskers commented Jul 5, 2022 • edited

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann Jul 5, 2022

Choose a reason for hiding this comment

kevinrenskers Jul 5, 2022

Choose a reason for hiding this comment

kevinrenskers Jul 5, 2022

Choose a reason for hiding this comment

philipphofmann commented Jul 5, 2022

kevinrenskers commented Jul 5, 2022

kevinrenskers commented Jul 6, 2022

kevinrenskers commented Jul 6, 2022

philipphofmann commented Jul 6, 2022

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

kevinrenskers commented Jul 4, 2022 •

edited by philipphofmann

github-actions bot commented Jul 4, 2022 •

edited

kevinrenskers commented Jul 5, 2022 •

edited