getsentry
Replace Device.Timezone property
There's currently a TODO item in Sentry.Protocol.Device added by Alexey here.
https://github.com/getsentry/sentry-dotnet/blob/e75d537c14ded617c99eb33f19cf4ce69ca4de36/src/Sentry/Protocol/Device.cs#L18-L26
A serialize -> deserialize can theoretically lose some information at the moment. See: https://github.com/getsentry/sentry-dotnet/blob/e6c1d6491a7b2af9f1d0e1d0944b33778cb3b8fe/test/Sentry.Tests/Protocol/Context/DeviceTests.cs#L199-L243
This is likely only a problem with custom time zones. We haven't yet bumped into anyone using these. So we agreed not to address this until/unless it becomes a problem for one of our customers.
I believe he's talking about deserialization. If we serialize->deserialize->serialize again (happens when offline mode is turn on where we go to disk then back to memory then to the network).
Does it work then?
I believe he's talking about deserialization. If we serialize->deserialize->serialize again (happens when offline mode is turn on where we go to disk then back to memory then to the network).
Does it work then?
OK, that's a bit more concrete. That gives us something to test then.
Although, as this seems to be coming through to Sentry.io just fine at the moment, I'm not sure why we'd change it (if it ain't broke, don't fix it).
Let's move out of the 4.0.0 milestone then?
I believe he's talking about deserialization. If we serialize->deserialize->serialize again (happens when offline mode is turn on where we go to disk then back to memory then to the network).
In theory, even just a serialize -> deserialize will lose some information at the moment. See: https://github.com/getsentry/sentry-dotnet/blob/e6c1d6491a7b2af9f1d0e1d0944b33778cb3b8fe/test/Sentry.Tests/Protocol/Context/DeviceTests.cs#L199-L243
As far as I can tell, this would only be a problem with custom time zones, however... so whilst it's a theoretical problem, I'm not sure how likely it is that it would ever be an actual problem that one of our SDK customers runs into. Do we know of anyone, ever, who needed to use a custom time zone for some reason?
I'd suggest postponing this until we had at least one SDK customer asking for it.
I'd suggest postponing this until we had at least one SDK customer asking for it.
I agree. This looks to me like a very specific optimization. I'll remove it from the 5.0 milestone and drop the priority.
As is and with the scope of the changes needed, I'd suggest removing the TODO from the code (it will be kept here because of the permalink in the issue description so it's not lost) and closing the issue.
I'd suggest removing the TODO from the code
Done. Only 522 todo items to go!