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

Send device locale with Events #1438

Closed
philipphofmann opened this issue Oct 29, 2021 · 9 comments · Fixed by #1539
Closed

Send device locale with Events #1438

philipphofmann opened this issue Oct 29, 2021 · 9 comments · Fixed by #1539
Assignees

Comments

@philipphofmann
Copy link
Member

Discover lists device.locale as a search condition, see https://docs.sentry.io/product/discover-queries/query-builder/. The develop docs list the locale in culture context, see https://develop.sentry.dev/sdk/event-payloads/contexts/#culture-context. Figure out how to send the device locale so customers can use it in Discover. After this maybe fix the Discover or Develop docs and create an issue for the Java repo.

@marandaneto
Copy link
Contributor

marandaneto commented Nov 8, 2021

Android offers device.language with the eg en_US format, and device.timezone with the eg Europe/Paris format, but no device.locale either.
it'd make sense to add the culture context then https://develop.sentry.dev/sdk/event-payloads/contexts/#culture-context on Android too, maybe deprecating both fields from device for a while since people can rely on.

@philipphofmann
Copy link
Member Author

Sync with getsentry/sentry#29879.

@philipphofmann philipphofmann self-assigned this Nov 17, 2021
@philipphofmann
Copy link
Member Author

philipphofmann commented Dec 7, 2021

None of our events in our Sentry main project has a device.locale. I didn't find any event with device.locale in a quick check of a few big customers. As the device.locale isn't documented in the develop docs, I assume no SDKs are sending it. As device.locale already exists in Discover and there is a column in Snuba, we should add locale to the dev docs device context.

Todos:

Culture Conext

  • Removedevice.locale in Cocoa SDK 8.x
  • Add culture_context to the Cocoa SDK
  • Add culture_context to Android 6.x
  • Mark devicel.locale as deprecated on Android 6.x
  • Discover should work with culture_context

@bruno-garcia
Copy link
Member

As the device.locale isn't documented in the develop docs

Turns out we added this under a new Context called culture.

image

Fix the description of the language of the dev docs of device context. Instead of en-US change it to en.

This is a breaking change since these values are indexed and used to show distribution and potentially alerts. Do we need to change from one to the other? If so, what ISO format are we going from, to?

@marandaneto
Copy link
Contributor

Find out if we why language and locale are duplicate for device and culture context

culture_context is a new thing because of flutter, it didn't make sense to bloat the context.device with so much stuff, so it made sense to create a new one.
following the new format, device.locale, device.language, and device.timezone should be deprecated in favor of culture_context.
the problem is that this is a bigger change and requires bumping a major etc, I'd rather add the missing fields on iOS for now for a quick win.

@philipphofmann
Copy link
Member Author

philipphofmann commented Dec 9, 2021

following the new format, device.locale, device.language, and device.timezone should be deprecated in favor of culture_context.

Who said that ⬆️ ?

I don't think culture_context is already indexed and you can use it in Discover.

@marandaneto
Copy link
Contributor

@philipphofmann that was just the natural way going forward after adding culture_context, otherwise it'd give duplicated fields, see getsentry/develop#329
its part of the unified API now for a while, but I guess only Dart and .NET have it iirc.

culture_context isn't indexed, yes, it should going forward.

@philipphofmann
Copy link
Member Author

Why don't the develop docs then state that they are deprecated? It doesn't help to decide this in the PR and not putting this in the docs. If the official decision is they are deprecated we should communicate that on the docs.

culture_context isn't indexed, yes, it should going forward.

Sorry, I don't get that. Are you saying we should index it? If so, deciding in getsentry/develop#329 to use culture_context and not making sure they are indexed is not nice. We have to fix that.

@marandaneto
Copy link
Contributor

@philipphofmann please invite us for a call, its easier

philipphofmann added a commit that referenced this issue Dec 10, 2021
Send the current locale with events. The SDK puts the locale into the device
context of the event.

Fixes GH-1438
philipphofmann added a commit that referenced this issue Dec 10, 2021
Send the current locale with events. The SDK puts the locale into the device
context of the event.

Fixes GH-1438
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants