-
-
Notifications
You must be signed in to change notification settings - Fork 239
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
Update protocol #1038
Update protocol #1038
Conversation
Codecov ReportBase: 90.45% // Head: 91.37% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1038 +/- ##
==========================================
+ Coverage 90.45% 91.37% +0.91%
==========================================
Files 114 9 -105
Lines 3584 197 -3387
==========================================
- Hits 3242 180 -3062
+ Misses 342 17 -325 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@@ -0,0 +1,29 @@ | |||
/// Geographical location of the end user or device. | |||
class SentryGeo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is inferred from ip_address
in the server, but if its set, it keeps as it is, I'm checking if SDKs should be allowed to set their own geo
data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it, I will update the develop docs.
We have to fix the Native bridges, to also sync the geo
data.
Such as Android:
sentry-dart/flutter/android/src/main/kotlin/io/sentry/flutter/SentryFlutterPlugin.kt
Lines 265 to 291 in e390ced
private fun setUser(user: Map<String, Any?>?, result: Result) { | |
if (user == null) { | |
Sentry.setUser(null) | |
result.success("") | |
return | |
} | |
val userInstance = User() | |
(user["email"] as? String)?.let { userInstance.email = it } | |
(user["id"] as? String)?.let { userInstance.id = it } | |
(user["username"] as? String)?.let { userInstance.username = it } | |
(user["ip_address"] as? String)?.let { userInstance.ipAddress = it } | |
(user["extras"] as? Map<String, Any?>)?.let { extras -> | |
val others = mutableMapOf<String, String>() | |
for ((key, value) in extras.entries) { | |
if (value != null) { | |
others[key] = value.toString() | |
} | |
} | |
userInstance.others = others | |
} | |
Sentry.setUser(userInstance) | |
result.success("") | |
} |
iOS
sentry-dart/flutter/ios/Classes/SentryFlutterPluginApple.swift
Lines 503 to 528 in e390ced
private func setUser(user: [String: Any?]?, result: @escaping FlutterResult) { | |
if let user = user { | |
let userInstance = User() | |
if let email = user["email"] as? String { | |
userInstance.email = email | |
} | |
if let id = user["id"] as? String { | |
userInstance.userId = id | |
} | |
if let username = user["username"] as? String { | |
userInstance.username = username | |
} | |
if let ipAddress = user["ip_address"] as? String { | |
userInstance.ipAddress = ipAddress | |
} | |
if let extras = user["extras"] as? [String: Any] { | |
userInstance.data = extras | |
} | |
SentrySDK.setUser(userInstance) | |
} else { | |
SentrySDK.setUser(nil) | |
} | |
result("") | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do, but why is it this cumbersome to set the user on the native layer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both, Cocoa and Java, don't have the Geo
object yet. I've added the data sync though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getsentry/team-mobile#59 (comment)
Added a comment on the geo
field on Dart that this is not going to be synced for now (scope sync).
I will see if I can get this done on both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe we add it under data
for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went for the solution of documenting the missing sync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Manoel Aranda Neto <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a comment.
There seems to be an issue with the NDK installation, do you know why that's happening, @marandaneto?
|
I suspect sentry-dart/flutter/example/android/app/build.gradle Lines 67 to 69 in 56810ff
Maybe GH action images changed, since this is the sample, feel free to either remove it or use a version that works, as long as CI is happy. |
Ci is now happy 🎉 |
@@ -4,6 +4,7 @@ | |||
|
|||
### Fixes | |||
|
|||
- Bring protocol up to date with latest Sentry protocol ([#1038](https://github.com/getsentry/sentry-dart/pull/1038)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to move to unreleased section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it. You're doing way too many releases lately, I can barely keep up with it :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @ueman
📜 Description
Update protocol. I've also cleaned up some more
toJson
methods.💡 Motivation and Context
Fixes #1028
💚 How did you test it?
📝 Checklist
🔮 Next steps