Native crumbs by melekr · Pull Request #111 · backtrace-labs/backtrace-cocoa

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

Remove Cassette dependency
Replace Cassette Queue and file operations

Remove Cassette related Tests

KrzaQ

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

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

-        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?

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

-        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.

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

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

+
+    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

-                        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

@@ -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

@@ -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

                        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

                        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

-                        // 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

  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

Comment thread Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift Outdated

melekr commented Apr 28, 2023

View reviewed changes

Comment thread Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift Outdated

rick-bt reviewed Apr 28, 2023

View reviewed changes

Comment thread Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift Outdated

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

melekr commented Apr 28, 2023

View reviewed changes

Comment thread Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift Outdated

KrzaQ approved these changes Apr 28, 2023

View reviewed changes

Comment thread Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift Outdated

Comment thread Sources/Features/Breadcrumb/BacktraceBreadcrumbFileHelper.swift Outdated

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()

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

Uh oh!

Reviewers

Assignees

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