Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Deserializing Events #4724

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from
Draft

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Jan 16, 2025

📜 Description

Add Decodable implementations via Swift extensions to deserialize SentryEvents.

This is the feature branch PR for deserializing SentryEvents. We're going to open multiple PRs to this one, so we have smaller PRs for quicker reviews.

To minimize the scope the goal is not to use Encodable and keep the existing serialization logic.

💡 Motivation and Context

We need this for app hangs #4261.

💚 How did you test it?

📝 Checklist

You have to check all boxes before merging:

  • 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.
  • I updated the wizard 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.

🔮 Next steps

Add Decodable implementations via Swift extensions to deserialize
SentryEvents.
Copy link

github-actions bot commented Jan 16, 2025

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

Generated by 🚫 dangerJS against 8067173

@philprime
Copy link
Contributor

In case you want to implement a custom encoder for converting to the dictionary, I did implement a custom encoder before in Postie, to parse custom property wrappers/annotations into a structure to send URL requests.

Even though that use case is quite more complicated than just encoding to a dictionary, it might be helpful for reference:

https://github.com/kula-app/Postie/blob/main/Sources/Postie/Encoder/RequestEncoder.swift

Copy link

github-actions bot commented Jan 16, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1223.78 ms 1235.52 ms 11.75 ms
Size 22.31 KiB 805.86 KiB 783.54 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
1a86fb2 1224.69 ms 1236.47 ms 11.77 ms
72c8d84 1218.69 ms 1241.08 ms 22.39 ms
c76f9b0 1249.78 ms 1265.14 ms 15.36 ms
7cd187e 1243.56 ms 1250.20 ms 6.64 ms
ed1c644 1225.92 ms 1241.24 ms 15.33 ms
881a955 1214.27 ms 1235.88 ms 21.61 ms
fb53d97 1235.00 ms 1241.88 ms 6.88 ms
3dcba20 1237.14 ms 1249.69 ms 12.54 ms
b9fc537 1211.00 ms 1220.40 ms 9.40 ms
313b1d9 1240.18 ms 1258.44 ms 18.26 ms

App size

Revision Plain With Sentry Diff
1a86fb2 21.58 KiB 612.11 KiB 590.53 KiB
72c8d84 22.85 KiB 408.88 KiB 386.03 KiB
c76f9b0 22.85 KiB 406.69 KiB 383.84 KiB
7cd187e 20.76 KiB 401.65 KiB 380.89 KiB
ed1c644 21.58 KiB 670.39 KiB 648.81 KiB
881a955 22.85 KiB 407.63 KiB 384.78 KiB
fb53d97 20.76 KiB 425.80 KiB 405.04 KiB
3dcba20 22.31 KiB 780.90 KiB 758.58 KiB
b9fc537 21.58 KiB 676.19 KiB 654.61 KiB
313b1d9 22.85 KiB 414.11 KiB 391.26 KiB

Previous results on branch: feat/deserializing-events

Startup times

Revision Plain With Sentry Diff
242b57f 1236.13 ms 1252.17 ms 16.04 ms
74e111a 1229.31 ms 1244.63 ms 15.33 ms
378d4bd 1216.53 ms 1241.19 ms 24.66 ms
04e8394 1223.24 ms 1244.33 ms 21.08 ms
4dc265c 1221.31 ms 1238.55 ms 17.24 ms
8a562cc 1228.57 ms 1245.18 ms 16.60 ms
8dae1f6 1212.60 ms 1226.52 ms 13.93 ms

App size

Revision Plain With Sentry Diff
242b57f 22.31 KiB 801.48 KiB 779.17 KiB
74e111a 22.31 KiB 800.12 KiB 777.80 KiB
378d4bd 22.31 KiB 784.05 KiB 761.73 KiB
04e8394 22.31 KiB 771.93 KiB 749.62 KiB
4dc265c 22.31 KiB 798.21 KiB 775.90 KiB
8a562cc 22.31 KiB 773.33 KiB 751.02 KiB
8dae1f6 22.31 KiB 772.51 KiB 750.20 KiB

@philipphofmann
Copy link
Member Author

In case you want to implement a custom encoder for converting to the dictionary

Thanks for pointing that out. I would rather spend the time converting all the serialization to Decoable, because that's what we should do on the long run, IMO.

@philprime, does the overall approach look good to you?

@philprime
Copy link
Contributor

Looks good to me.

@philipphofmann
Copy link
Member Author

philipphofmann commented Jan 16, 2025

FYI, @philprime, I ditched the Encodable because we have some tests validating the data for envelopes that break when using Encodable. Switching the whole serialization to Encodable will make fixing the tests easier. I want to keep the scope small for now.

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 99.61832% with 5 lines in your changes missing coverage. Please review.

Project coverage is 91.436%. Comparing base (c6a8035) to head (8067173).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...s/Swift/Protocol/Codable/DecodeArbitraryData.swift 92.957% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4724       +/-   ##
=============================================
+ Coverage   91.398%   91.436%   +0.038%     
=============================================
  Files          627       645       +18     
  Lines        74571     75709     +1138     
  Branches     26810     26517      -293     
=============================================
+ Hits         68157     69226     +1069     
- Misses        6315      6390       +75     
+ Partials        99        93        -6     
Files with missing lines Coverage Δ
Sources/Sentry/SentryGeo.m 95.000% <100.000%> (+1.000%) ⬆️
Sources/Sentry/SentryLevelHelper.m 100.000% <100.000%> (ø)
...ft/Protocol/Codable/NSNumberDecodableWrapper.swift 100.000% <100.000%> (ø)
...ift/Protocol/Codable/SentryBreadcrumbCodable.swift 100.000% <100.000%> (ø)
Sources/Swift/Protocol/Codable/SentryCodable.swift 100.000% <100.000%> (ø)
...wift/Protocol/Codable/SentryDebugMetaCodable.swift 100.000% <100.000%> (ø)
...wift/Protocol/Codable/SentryExceptionCodable.swift 100.000% <100.000%> (ø)
...es/Swift/Protocol/Codable/SentryFrameCodable.swift 100.000% <100.000%> (ø)
...rces/Swift/Protocol/Codable/SentryGeoCodable.swift 100.000% <100.000%> (ø)
...wift/Protocol/Codable/SentryMechanismCodable.swift 100.000% <100.000%> (ø)
... and 24 more

... and 42 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 c6a8035...8067173. Read the comment docs.

Comment on lines 3 to 15
func decodeFromJSONData<T: Decodable>(jsonData: Data) -> T? {
if jsonData.isEmpty {
return nil
}

do {
let decoder = JSONDecoder()
return try decoder.decode(T.self, from: jsonData)
} catch {
SentryLog.error("Could not decode object of type \(T.self) from JSON data due to error: \(error)")
return nil
}
}
Copy link
Contributor

@brustolin brustolin Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l:

Suggested change
func decodeFromJSONData<T: Decodable>(jsonData: Data) -> T? {
if jsonData.isEmpty {
return nil
}
do {
let decoder = JSONDecoder()
return try decoder.decode(T.self, from: jsonData)
} catch {
SentryLog.error("Could not decode object of type \(T.self) from JSON data due to error: \(error)")
return nil
}
}
extension Decodable {
init?(jsonData: Data) {
guard !jsonData.isEmpty else {
return nil
}
do {
let decoder = JSONDecoder()
self = try decoder.decode(Self.self, from: jsonData)
} catch {
print("Could not decode object of type \(Self.self) from JSON data due to error: \(error)")
return nil
}
}
}

Then, instead of

let decoded = decodeFromJSONData(jsonData: data) as Geo?

We have

let decoded = Geo(jsonData: data)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to point out that Decodable is considered to be decoder-agnostic, so adding a convenience method to the type would not be agnostic anymore. It also takes away the option to configure JSONDecoder in unit tests etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is just a convenience initializer, just like the decodeFromJSONData that it would replace, that also does not give the option to configure JSONDecoder

Add Decodable/Deserializing of SentryFrame, including logic for decoding NSNumbers.
Add Decodable/Deserializing of SentryMessage.
Add Decodable/Deserializing of SentryDebugMeta.
Add Decodable/Deserializing of SentryThread.
Add Decodable/Deserializing of SentryException.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants