Native crumbs #111

melekr · 2023-04-28T20:03:54Z

Remove Cassette dependency
Replace Cassette Queue and file operations

Remove Cassette related Tests

Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift

KrzaQ

I don't have much to say, it looks ok to me.

Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift

konraddysput · 2023-05-01T17:31:50Z

Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift

-        self.queue = try CASQueueFile.init(path: breadcrumbSettings.getBreadcrumbLogPath().path)
-
+
+        self.breadcrumbLogURL = try breadcrumbSettings.getBreadcrumbLogPath()


why it can throw? Maybe it would be better to throw an error faster and here pass only data needed by BacktraceBreadcrumbFileHelper?

Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift

konraddysput · 2023-05-01T17:41:55Z

Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift

-        self.queue = try CASQueueFile.init(path: breadcrumbSettings.getBreadcrumbLogPath().path)
-
+
+        self.breadcrumbLogURL = try breadcrumbSettings.getBreadcrumbLogPath()


what if the file doesn't exist? we don't create it anywhere. We always write to a file. How about creating a start method that will do startup logic?

the file will always exist;
getBreadcrumbLogPath() method will create a file if doesn't exist

there is no good way to do this since BacktraceBreadcrumbSettings forces us to (try/unwrap), if for any reason the file is nil the files operations methods will catch the error and warn BacktraceLogger.

Sources/Features/Breadcrumb/QueueFile.swift

Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift

Sources/Features/Breadcrumb/QueueFile.swift

Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift

Rename BacktraceBreadcrumbFileHelper to BacktraceBreadcrumbFile Code improvements

Add BacktraceBreadcrumbFile extension to host file operations Add remove(at index: Int) to Queue class Update BacktraceLogger error messages

Sources/Features/Breadcrumb/BacktraceBreadcrumbFile.swift

konraddysput · 2023-05-04T21:36:44Z

Sources/Features/Breadcrumb/BacktraceBreadcrumbFile.swift

+
+    func clearBreadcrumbLogFile(at breadcrumbLogURL: URL) {
+        do {
+            try "".write(to: breadcrumbLogURL, atomically: false, encoding: .utf8)


what if someone deletes a file when you're generating breadcrumbs?

konraddysput · 2023-05-05T09:47:52Z

Sources/Features/Breadcrumb/BacktraceBreadcrumbFile.swift

-                        BacktraceLogger.warning("Error when adding breadcrumbSize to array")
-                        return false
-                    }
+                    let queueBreadcrumb = queuedBreadcrumbs[index]


We receive index out of bounds when we access breadcrumbs with an invalid index, right? or will it return null?

it shouldn't but I'll guard it

konraddysput · 2023-05-05T09:48:35Z

Sources/Features/Breadcrumb/QueueFile.swift

@@ -1,5 +1,15 @@
 import Foundation

+struct BreadcrumbRecord {


I think it should be moved to a separated file.
You implemented queue to be generic. However, putting BreadcrumbRecord definition in the Queue implementation sounds weird.

konraddysput · 2023-05-05T09:48:59Z

Sources/Features/Breadcrumb/QueueFile.swift

@@ -1,5 +1,15 @@
 import Foundation


Is it QueueFile? I believe its a generic queue instead. Can we rename the file to Queue or something like that?

Queue it is!

Add BreadcrumbRecord class definition Rename QueueFile to Queue Guard queue index when iterating

vlussenburg · 2023-05-08T21:44:53Z

Tests/BacktraceBreadcrumbTests.swift

                        let text = "this is Breadcrumb number \(writeIndex)"
-                        // submit a task to the queue for background execution
-                        DispatchQueue.global().async(group: group, execute: {
-                            expect { breadcrumbs.addBreadcrumb(text) }.to(beTrue())


Without this background task, this test doesn't do what is intended to do (in fact, the entire loop is useless).

We had lots of problems with multithreaded behavior corrupting the file, so beware!

I'll seep a close eye on multithreaded behaviour, thank you!

vlussenburg · 2023-05-08T21:45:49Z

Tests/BacktraceBreadcrumbTests.swift

                        let fileSize = attr[FileAttributeKey.size] as? Int
                        let requestedSize = settings.maxQueueFileSizeBytes
                        expect { fileSize }.to(beLessThanOrEqualTo(requestedSize))
-                        expect { fileSize }.to(beGreaterThanOrEqualTo(requestedSize - 1000))


This check is important. I would consider adjusting it, not deleting it.

vlussenburg · 2023-05-08T21:45:54Z

Tests/BacktraceBreadcrumbTests.swift

-                        // Currently, we accept we lose this Breadcrumb in the UI - it will still be in the file
-                        // for manual inspection.
-                        let expectedNumberOfMatches = writeIndex - wrapIndex - 1
-                        expect(matches).to(beGreaterThanOrEqualTo(expectedNumberOfMatches),


This check is important. I would consider adjusting it, not deleting it. The -1 may no longer be necessary due to the Cassette dependency being removed.

This test is specifically there to make sure the file rolls over, so the number of expected matches are really important. Otherwise the entire test doesn't do anything the other (smaller) tests already do.

Removed this one for the same reason, I'll adjust it.
Thank you :)

vlussenburg · 2023-05-08T21:49:29Z

Backtrace.podspec

  s.osx.public_header_files = ["Backtrace-macOS/**/*.h*"]
  s.tvos.public_header_files = ["Backtrace-tvOS/**/*.h*"]

-  s.ios.dependency "Cassette", '1.0.0-beta5'


Check if there's any superfluous #if os(iOS) || os(OSX) left in prod/test code. I know there were a bunch of 'm

I'll take a look :)

vlussenburg · 2023-05-08T21:51:56Z

Hey hey!

Cool that that pesky Cassette is finally being removed :). I forgot where, but there's performance test results stored in Github or Hack somewhere that document for which file sizes the performance is what.

This PR seems to be doing somewhat similar to what #95 was meant to do (but is outdated now). You may want to compare and then close it.

Thanks :)
Vincent

melekr added 2 commits April 28, 2023 16:01

Remove Cassette dependency

aaf7aa6

Implement swift breadcrumbs

1d3d5b1

Remove Cassette related Tests

melekr requested review from KrzaQ, konraddysput and rick-bt April 28, 2023 20:04

melekr commented Apr 28, 2023

View reviewed changes

Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift Outdated Show resolved Hide resolved

melekr commented Apr 28, 2023

View reviewed changes

Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift Outdated Show resolved Hide resolved

rick-bt reviewed Apr 28, 2023

View reviewed changes

Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift Outdated Show resolved Hide resolved

rick-bt requested a review from ianrice07 April 28, 2023 20:39

melekr commented Apr 28, 2023

View reviewed changes

Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift Outdated Show resolved Hide resolved

KrzaQ approved these changes Apr 28, 2023

View reviewed changes

Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift Outdated Show resolved Hide resolved

Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift Outdated Show resolved Hide resolved

konraddysput reviewed May 1, 2023

View reviewed changes

melekr added 2 commits May 3, 2023 16:38

Update BacktraceBreadcrumbFile class

709c549

Rename BacktraceBreadcrumbFileHelper to BacktraceBreadcrumbFile Code improvements

Synchronize addBreadcrumb operation

3d09e17

Add BacktraceBreadcrumbFile extension to host file operations Add remove(at index: Int) to Queue class Update BacktraceLogger error messages

konraddysput reviewed May 4, 2023

View reviewed changes

konraddysput approved these changes May 4, 2023

View reviewed changes

melekr added 2 commits May 4, 2023 19:02

Add BreadcrumbRecord

d66875a

Remove extra do & catch statements

d8ece9d

konraddysput approved these changes May 5, 2023

View reviewed changes

melekr added 2 commits May 5, 2023 10:52

Code cleanup

f592065

Add BreadcrumbRecord class definition Rename QueueFile to Queue Guard queue index when iterating

Update project.pbxproj

e851ab7

vlussenburg reviewed May 8, 2023

View reviewed changes

melekr closed this May 11, 2023

		self.queue = try CASQueueFile.init(path: breadcrumbSettings.getBreadcrumbLogPath().path)


		self.breadcrumbLogURL = try breadcrumbSettings.getBreadcrumbLogPath()

Native crumbs #111

Native crumbs #111

Uh oh!

Conversation

melekr commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KrzaQ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlussenburg May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlussenburg May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlussenburg May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlussenburg commented May 8, 2023

melekr commented Apr 28, 2023 •

edited

Loading

vlussenburg May 8, 2023 •

edited

Loading

vlussenburg May 8, 2023 •

edited

Loading

vlussenburg May 8, 2023 •

edited

Loading