chore(internal): Add `packages` set to SentrySdkInfo #4598

krystofwoldrich · 2024-12-04T14:23:48Z

📜 Description

This PR adds packages to Sentry Envelope Header SDK Info and to Sentry Event SDK Info.

Before this PR packages were not set in the Event and in Header the Cocoa package was hardcoded.

💡 Motivation and Context

This will be used by hybrid SDKs to add itself as package, later we can use this information during debugging SR and other events.

💚 How did you test it?

By sending events from Sentry SwiftUI sample and unit tests.

📝 Checklist

You have to check all boxes before merging:

I reviewed the submitted code.
I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

…ges-to-sdk-info

github-actions · 2024-12-04T15:15:05Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1217.10 ms	1246.40 ms	29.30 ms
Size	22.30 KiB	752.19 KiB	729.89 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`187edbf`	1224.29 ms	1239.69 ms	15.40 ms
`2095ae0`	1238.20 ms	1251.37 ms	13.17 ms
`df2835d`	1218.78 ms	1238.35 ms	19.57 ms
`8c9d1d4`	1239.13 ms	1253.00 ms	13.87 ms
`62c15d4`	1235.30 ms	1260.82 ms	25.52 ms
`e5ac362`	1204.12 ms	1229.41 ms	25.29 ms
`72c8d84`	1238.96 ms	1247.34 ms	8.38 ms
`fcde045`	1260.71 ms	1275.00 ms	14.29 ms
`e3adcd5`	1206.47 ms	1215.39 ms	8.92 ms
`42ef6ba`	1195.04 ms	1214.35 ms	19.31 ms

App size

Revision	Plain	With Sentry	Diff
`187edbf`	21.58 KiB	729.90 KiB	708.32 KiB
`2095ae0`	21.90 KiB	709.06 KiB	687.16 KiB
`df2835d`	21.58 KiB	706.14 KiB	684.55 KiB
`8c9d1d4`	21.58 KiB	706.97 KiB	685.39 KiB
`62c15d4`	22.85 KiB	411.14 KiB	388.29 KiB
`e5ac362`	20.76 KiB	426.11 KiB	405.34 KiB
`72c8d84`	22.85 KiB	408.88 KiB	386.03 KiB
`fcde045`	20.76 KiB	435.26 KiB	414.50 KiB
`e3adcd5`	21.58 KiB	539.87 KiB	518.28 KiB
`42ef6ba`	21.58 KiB	417.87 KiB	396.28 KiB

Previous results on branch: kw/internal-add-packages-to-sdk-info

Startup times

Revision	Plain	With Sentry	Diff
`10894a6`	1228.14 ms	1248.37 ms	20.22 ms
`1d36029`	1236.36 ms	1257.19 ms	20.83 ms
`dc421e2`	1239.73 ms	1251.85 ms	12.13 ms

App size

Revision	Plain	With Sentry	Diff
`10894a6`	22.30 KiB	752.13 KiB	729.83 KiB
`1d36029`	22.30 KiB	752.19 KiB	729.89 KiB
`dc421e2`	22.30 KiB	752.19 KiB	729.89 KiB

…info' into kw/internal-add-packages-to-sdk-info

krystofwoldrich · 2024-12-05T10:52:41Z

Sources/Sentry/SentrySdkInfo.m

-            sdk[@"packages"] = @{
-                @"name" : [NSString stringWithFormat:format, self.name],
-                @"version" : self.version
-            };


Based on the development docs this should be an array, I don't know if this value was used anywhere, so I just updated it to the array.

If it's not used I would consider removing it completely and only create packages in SentryClient (same as features, integrations...).

Headers docs: https://develop.sentry.dev/sdk/data-model/envelopes/#envelope-headers

SDK Info interface docs: https://develop.sentry.dev/sdk/data-model/event-payloads/sdk/

codecov · 2024-12-05T10:55:26Z

Codecov Report

Attention: Patch coverage is 96.36364% with 10 lines in your changes missing coverage. Please review.

Project coverage is 91.025%. Comparing base (963b49c) to head (3b3e844).

Files with missing lines	Patch %	Lines
Sources/Sentry/SentrySdkPackage.m	93.103%	4 Missing ⚠️
Sources/Sentry/SentrySdkPackage+Equality.m	90.000%	1 Missing and 1 partial ⚠️
...ests/SentryTests/Protocol/SentrySdkInfo+Equality.m	50.000%	2 Missing ⚠️
Sources/Sentry/SentrySdkInfo.m	96.774%	1 Missing ⚠️
...ests/SentryTests/Protocol/SentrySdkInfoTests.swift	98.809%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #4598       +/-   ##
=============================================
+ Coverage   90.978%   91.025%   +0.047%     
=============================================
  Files          616       620        +4     
  Lines        70785     71159      +374     
  Branches     25173     25419      +246     
=============================================
+ Hits         64399     64773      +374     
+ Misses        6293      6292        -1     
- Partials        93        94        +1

Files with missing lines	Coverage Δ
Sources/Sentry/PrivateSentrySDKOnly.mm	`89.447% <100.000%> (+0.161%)`	⬆️
Sources/Sentry/SentryClient.m	`98.703% <100.000%> (+0.001%)`	⬆️
Sources/Sentry/SentryEnvelope.m	`90.625% <100.000%> (+0.049%)`	⬆️
Sources/Sentry/SentryMeta.m	`100.000% <100.000%> (ø)`
Tests/SentryTests/PrivateSentrySDKOnlyTests.swift	`99.756% <100.000%> (+0.003%)`	⬆️
Tests/SentryTests/Protocol/SentryMetaTests.swift	`100.000% <100.000%> (ø)`
Tests/SentryTests/SentryClientTests.swift	`97.840% <100.000%> (+0.012%)`	⬆️
Sources/Sentry/SentrySdkInfo.m	`90.625% <96.774%> (-1.042%)`	⬇️
...ests/SentryTests/Protocol/SentrySdkInfoTests.swift	`97.005% <98.809%> (+0.815%)`	⬆️
Sources/Sentry/SentrySdkPackage+Equality.m	`90.000% <90.000%> (ø)`
... and 2 more

... and 30 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 963b49c...3b3e844. Read the comment docs.

krystofwoldrich · 2024-12-05T10:57:26Z

This seems like a lot of code for a small change, please when review let me know, if I've overlooked something and there is better place to save the packages to.

philipphofmann

I found a view high-level issues. We need to address them before I give this a more detailed review. @krystofwoldrich we can also discuss this on a call.

philipphofmann · 2024-12-06T08:32:51Z

Sources/Sentry/SentryEnvelope.m

+                                                      andVersion:SentryMeta.versionString
+                                                     andPackages:[SentryMeta getSdkPackages]];


h: The SDKInfo of the envelope header is the same as the one from the event payload: see

This can carry the same payload as the sdk interface
in the event payload but can be carried for all events. This means that SDK
information can be carried for minidumps, session data and other submissions. - https://develop.sentry.dev/sdk/data-model/envelopes/#envelope-headers

Now that we touch both SDKInfo for the envelope header and the event payload, I think it would be great to merge the different implementations. To make this easier we could do this in an extra PR before adding the new packages implementation.

philipphofmann · 2024-12-06T08:34:20Z

Sources/Sentry/include/SentryMeta.h

+/**
+ * Return an array of Serialized SDK packages
+ */
+ (NSArray<NSDictionary<NSString *, NSString *> *> *)getSdkPackagesSerialized;


m: I don't think the SDK packages really belong here. The SentryMeta is only about version and SDK name. getSdkPackagesSerialized is a sign for me that it doesn't belong here. Instead, I would create an extra class just for the SDK packages. You can even write it in Swift.

philipphofmann · 2024-12-06T08:35:44Z

Sources/Sentry/SentrySdkPackage.m

+#import "SentryMeta.h"
+#import <Foundation/Foundation.h>
+
+typedef NS_ENUM(NSUInteger, SentryPackageManagerOption) {


m: Can we convert this code to Swift, please?

philipphofmann · 2024-12-06T08:36:17Z

Sources/Sentry/SentrySdkPackage+Private.h

+#    import "SentrySdkPackage.h"
+#endif
+
+@interface SentrySdkPackage ()


m: SentrySDKPackage is internal. Why did you have to add a +Private.h header?

philipphofmann · 2024-12-06T08:40:06Z

Sources/Sentry/SentrySdkPackage+Equality.h

+
+NS_ASSUME_NONNULL_BEGIN
+
+@interface SentrySdkPackage (Equality)


m: If we use a Swift struct for just the data of SentrySDKPackage you get isEqual for free.

…ges-to-sdk-info

github-actions · 2024-12-06T10:36:34Z

CHANGELOG.md

@@ -10,6 +10,10 @@

 - Track adoption of `enablePersistingTracesWhenCrashing` (#4587)

+### Internal
+
+- Add `packages` set to SentrySdkInfo (#4598)


🚫 The changelog entry seems to be part of an already released section ## 8.42.0.
Consider moving the entry to the ## Unreleased section, please.

krystofwoldrich added 2 commits December 4, 2024 15:23

chore(internal): Add packages set to SentrySdkInfo

10ebd39

Merge remote-tracking branch 'origin/main' into kw/internal-add-packa…

87299dd

…ges-to-sdk-info

krystofwoldrich and others added 7 commits December 4, 2024 17:29

add tests

a56cf84

Update CHANGELOG.md

2154692

Update Sentry.modulemap

2cc2664

Update Sentry.modulemap

b0bb91e

add private sdks api

158e3a8

Merge remote-tracking branch 'origin/kw/internal-add-packages-to-sdk-…

e0b0ce0

…info' into kw/internal-add-packages-to-sdk-info

fix format

0395d31

krystofwoldrich closed this Dec 4, 2024

krystofwoldrich reopened this Dec 4, 2024

krystofwoldrich added 3 commits December 4, 2024 18:32

fix format 2

29fe1d9

add packages to sentry client event sdk

0e2ca80

clean up after sentry meta test

e44ea4e

krystofwoldrich commented Dec 5, 2024

View reviewed changes

krystofwoldrich marked this pull request as ready for review December 5, 2024 10:55

krystofwoldrich requested review from philipphofmann, brustolin, armcknight and philprime as code owners December 5, 2024 10:55

philipphofmann reviewed Dec 6, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into kw/internal-add-packa…

3b3e844

…ges-to-sdk-info

github-actions bot reviewed Dec 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(internal): Add `packages` set to SentrySdkInfo #4598

chore(internal): Add `packages` set to SentrySdkInfo #4598

krystofwoldrich commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: kw/internal-add-packages-to-sdk-info

Startup times

App size

krystofwoldrich Dec 5, 2024 •

edited

Loading

codecov bot commented Dec 5, 2024 •

edited

Loading

krystofwoldrich commented Dec 5, 2024

philipphofmann left a comment

philipphofmann Dec 6, 2024

philipphofmann Dec 6, 2024

philipphofmann Dec 6, 2024

philipphofmann Dec 6, 2024

philipphofmann Dec 6, 2024

github-actions bot Dec 6, 2024

		andVersion:SentryMeta.versionString
		andPackages:[SentryMeta getSdkPackages]];


		NS_ASSUME_NONNULL_BEGIN

		@interface SentrySdkPackage (Equality)

chore(internal): Add packages set to SentrySdkInfo #4598

Are you sure you want to change the base?

chore(internal): Add packages set to SentrySdkInfo #4598

Conversation

krystofwoldrich commented Dec 4, 2024 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

github-actions bot commented Dec 4, 2024 • edited Loading

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: kw/internal-add-packages-to-sdk-info

Startup times

App size

krystofwoldrich Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Dec 5, 2024 • edited Loading

Codecov Report

krystofwoldrich commented Dec 5, 2024

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann Dec 6, 2024

Choose a reason for hiding this comment

philipphofmann Dec 6, 2024

Choose a reason for hiding this comment

philipphofmann Dec 6, 2024

Choose a reason for hiding this comment

philipphofmann Dec 6, 2024

Choose a reason for hiding this comment

philipphofmann Dec 6, 2024

Choose a reason for hiding this comment

github-actions bot Dec 6, 2024

Choose a reason for hiding this comment

chore(internal): Add `packages` set to SentrySdkInfo #4598

chore(internal): Add `packages` set to SentrySdkInfo #4598

krystofwoldrich commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

krystofwoldrich Dec 5, 2024 •

edited

Loading

codecov bot commented Dec 5, 2024 •

edited

Loading