feat: Add a timeout for auto-generated transactions by kevinrenskers · Pull Request #2535 · getsentry/sentry-cocoa

kevinrenskers · 2022-12-14T15:09:42Z

📜 Description

When we start the transaction we start a timer of 30 seconds, for auto generated transactions. When the timer reaches 0 then we mark the transaction and the unfinished child spans as deadline_exceeded and finish the transaction.

#skip-changelog

💡 Motivation and Context

Closes #1173

💚 How did you test it?

Unit tests

📝 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

Closes #1173

github-actions · 2022-12-14T15:11:04Z

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

Generated by 🚫 dangerJS against 5ca0e4f

kevinrenskers · 2022-12-14T15:11:30Z

This isn't really a user-facing feature that needs a changelog entry I guess? It's more of a safety net for Sentry.

github-actions · 2022-12-14T15:28:07Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1225.94 ms	1250.34 ms	24.40 ms
Size	20.75 KiB	405.15 KiB	384.39 KiB

Baseline results on branch: 8.0.0

Startup times

Revision	Plain	With Sentry	Diff
`0ccb7ce`	1244.24 ms	1245.16 ms	0.92 ms
`0c17d16`	1243.92 ms	1261.96 ms	18.04 ms
`f81c2c4`	1216.88 ms	1234.42 ms	17.54 ms
`010583c`	1198.23 ms	1225.52 ms	27.29 ms
`a9db8a6`	1199.83 ms	1234.56 ms	34.73 ms
`17e2418`	1245.59 ms	1255.52 ms	9.93 ms
`a8ac44f`	1198.63 ms	1231.08 ms	32.45 ms
`9be1db2`	1219.42 ms	1245.66 ms	26.24 ms
`57e140e`	1223.94 ms	1252.50 ms	28.56 ms
`d914052`	1236.52 ms	1251.56 ms	15.04 ms

App size

Revision	Plain	With Sentry	Diff
`0ccb7ce`	20.75 KiB	405.10 KiB	384.34 KiB
`0c17d16`	20.75 KiB	405.06 KiB	384.30 KiB
`f81c2c4`	20.75 KiB	402.12 KiB	381.36 KiB
`010583c`	20.75 KiB	383.61 KiB	362.85 KiB
`a9db8a6`	20.75 KiB	383.37 KiB	362.62 KiB
`17e2418`	20.75 KiB	402.06 KiB	381.31 KiB
`a8ac44f`	20.75 KiB	405.17 KiB	384.41 KiB
`9be1db2`	20.75 KiB	373.94 KiB	353.19 KiB
`57e140e`	20.75 KiB	383.61 KiB	362.85 KiB
`d914052`	20.75 KiB	383.89 KiB	363.13 KiB

Previous results on branch: feat/1173-timeout-transactions

Startup times

Revision	Plain	With Sentry	Diff
`514632f`	1246.86 ms	1266.40 ms	19.54 ms
`6d8af96`	1221.88 ms	1231.51 ms	9.63 ms
`ae95eaf`	1236.18 ms	1243.68 ms	7.50 ms
`b7e394e`	1221.86 ms	1232.43 ms	10.57 ms
`3ea4407`	1236.16 ms	1252.50 ms	16.34 ms
`1a5aac6`	1200.74 ms	1225.92 ms	25.18 ms
`ba761fd`	1203.73 ms	1224.46 ms	20.73 ms

App size

Revision	Plain	With Sentry	Diff
`514632f`	20.75 KiB	403.14 KiB	382.38 KiB
`6d8af96`	20.75 KiB	403.23 KiB	382.48 KiB
`ae95eaf`	20.75 KiB	405.15 KiB	384.40 KiB
`b7e394e`	20.75 KiB	403.24 KiB	382.48 KiB
`3ea4407`	20.75 KiB	403.14 KiB	382.38 KiB
`1a5aac6`	20.75 KiB	402.65 KiB	381.89 KiB
`ba761fd`	20.75 KiB	405.15 KiB	384.40 KiB

codecov · 2022-12-14T15:30:00Z

Codecov Report

Merging #2535 (a1901a9) into 8.0.0 (f81c2c4) will increase coverage by 0.25%.
The diff coverage is 96.22%.

❗ Current head a1901a9 differs from pull request most recent head 5ca0e4f. Consider uploading reports for the commit 5ca0e4f to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##            8.0.0    #2535      +/-   ##
==========================================
+ Coverage   78.81%   79.06%   +0.25%     
==========================================
  Files         241      244       +3     
  Lines       22144    22305     +161     
  Branches     9783     9858      +75     
==========================================
+ Hits        17453    17636     +183     
+ Misses       4245     4218      -27     
- Partials      446      451       +5

Impacted Files	Coverage Δ
Sources/Sentry/SentryNSTimerWrapper.m	`60.00% <60.00%> (ø)`
Sources/Sentry/SentryHub.m	`95.98% <100.00%> (+0.01%)`	⬆️
Sources/Sentry/SentryPerformanceTracker.m	`100.00% <100.00%> (ø)`
Sources/Sentry/SentryTracer.m	`95.36% <100.00%> (+1.19%)`	⬆️
Sources/Sentry/SentryAttachment.m	`92.85% <0.00%> (-7.15%)`	⬇️
Sources/Sentry/include/SentryLog.h	`80.00% <0.00%> (-4.22%)`	⬇️
Sources/Sentry/PrivateSentrySDKOnly.m	`91.17% <0.00%> (-4.21%)`	⬇️
Sources/Sentry/SentryBacktrace.cpp	`86.60% <0.00%> (-1.79%)`	⬇️
Sources/Sentry/SentryFileManager.m	`94.97% <0.00%> (-1.07%)`	⬇️
Sources/Sentry/SentryViewHierarchyIntegration.m	`88.88% <0.00%> (-0.86%)`	⬇️
... and 12 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 f81c2c4...5ca0e4f. Read the comment docs.

* 8.0.0: feat: support SENTRY_DSN environment var on macOS (#2534)

kevinrenskers · 2022-12-20T08:45:19Z

Can I get a review?

philipphofmann

Thanks for taking care of this, @kevinrenskers, and sorry for the late review.

Sources/Sentry/SentryTracer.m

Tests/SentryTests/Performance/SentryTracerTests.swift

philipphofmann · 2022-12-20T16:50:44Z

Sources/Sentry/SentryTracer.m

+    self.deadlineTimer = [self.timerWrapper
+        scheduledTimerWithTimeInterval:SENTRY_AUTO_TRANSACTION_DEADLINE
+                               repeats:NO
+                                 block:^(NSTimer *_Nonnull timer) { [self deadlineTimerFired]; }];


Please use a weak reference to self here. Also add a test for that, please. It can be similar to

sentry-cocoa/Tests/SentryTests/Helper/SentryFileManagerTests.swift

Lines 175 to 189 in 1a18683

func testFileManagerDeallocated_OldEnvelopesNotDeleted() throws {

try givenOldEnvelopes()

fixture.dispatchQueueWrapper.dispatchAsyncExecutesBlock = false

// Initialize sut in extra function so ARC deallocates it

func getSut() {

_ = fixture.getSut()

}

getSut()

fixture.dispatchQueueWrapper.invokeLastDispatchAsync()

XCTAssertEqual(sut.getAllEnvelopes().count, 1)

}

I've used a weak reference, but I don't know how you would want to add a test for that, the code you linked to doesn't really help as something similar doesn't work for the SentryTracer test case.

Tests/SentryTests/Performance/SentryTracerTests.swift

philipphofmann

Almost LGTM

Sources/Sentry/SentryTracer.m

Tests/SentryTests/Performance/SentryTracerTests.swift

kevinrenskers · 2023-01-07T17:05:07Z

Happy new year y'all!
I see this got auto-merged two days ago, even though there are some check failures, and I don't think it got an approval yet? You might want to look into why this got auto merged, but as far as I am aware, the PR was good to go.

philipphofmann · 2023-01-09T14:22:40Z

Happy new year to you, too, @kevinrenskers 😀. I just gave this another pass. LGTM 🙏 Thank you for following up on this one.

We missed adding #2535 to the Changelog of 8.0.0.

feat: Add a timeout for auto-generated transactions

29bde6c

Closes #1173

kevinrenskers requested review from armcknight, brustolin and philipphofmann as code owners December 14, 2022 15:09

kevinrenskers marked this pull request as draft December 14, 2022 19:02

Added a test

9a11b5e

kevinrenskers marked this pull request as ready for review December 15, 2022 12:31

kevinrenskers added 3 commits December 15, 2022 13:34

Merge branch '8.0.0' into feat/1173-timeout-transactions

c4853ea

* 8.0.0: feat: support SENTRY_DSN environment var on macOS (#2534)

Minimize the code changes

f16ac6a

Strengthen test

9f96b37

kevinrenskers enabled auto-merge (squash) December 20, 2022 11:25

philipphofmann reviewed Dec 20, 2022

View reviewed changes

kevinrenskers added 2 commits December 20, 2022 18:28

Use weak self, add more children to the test case

a251dc0

Add test

a1901a9

philipphofmann reviewed Dec 21, 2022

View reviewed changes

Added testDeadlineTimer_OnlyForAutoTransactions

5ca0e4f

kevinrenskers merged commit e0e432d into 8.0.0 Jan 5, 2023

philipphofmann deleted the feat/1173-timeout-transactions branch January 9, 2023 14:21

philipphofmann mentioned this pull request Jan 18, 2023

Timeout for auto-generated transactions #1173

Closed

philipphofmann added a commit that referenced this pull request Feb 14, 2023

chore: Add missing changelog for 2535

ab8f8b3

We missed adding #2535 to the Changelog of 8.0.0.

philipphofmann mentioned this pull request Feb 14, 2023

chore: Add missing changelog for 2535 #2690

Merged

brustolin pushed a commit that referenced this pull request Feb 15, 2023

chore: Add missing changelog for 2535 (#2690)

e9782e1

We missed adding #2535 to the Changelog of 8.0.0.

	func testFileManagerDeallocated_OldEnvelopesNotDeleted() throws {
	try givenOldEnvelopes()

	fixture.dispatchQueueWrapper.dispatchAsyncExecutesBlock = false

	// Initialize sut in extra function so ARC deallocates it
	func getSut() {
	_ = fixture.getSut()
	}
	getSut()

	fixture.dispatchQueueWrapper.invokeLastDispatchAsync()

	XCTAssertEqual(sut.getAllEnvelopes().count, 1)
	}

Uh oh!

Conversation

kevinrenskers commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

github-actions bot commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinrenskers commented Dec 14, 2022

Uh oh!

github-actions bot commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Baseline results on branch: 8.0.0

Startup times

App size

Previous results on branch: feat/1173-timeout-transactions

Startup times

App size

Uh oh!

codecov bot commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kevinrenskers commented Dec 20, 2022

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

philipphofmann Dec 20, 2022

Choose a reason for hiding this comment

Uh oh!

kevinrenskers Dec 20, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinrenskers commented Jan 7, 2023

Uh oh!

philipphofmann commented Jan 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kevinrenskers commented Dec 14, 2022 •

edited

Loading

github-actions bot commented Dec 14, 2022 •

edited

Loading

github-actions bot commented Dec 14, 2022 •

edited

Loading

codecov bot commented Dec 14, 2022 •

edited

Loading