feat(geo): Store subdivisions #2058
Conversation
Added 👍 |
relay-general/src/store/geo.rs
Outdated
| city.subdivisions | ||
| .as_ref() | ||
| .and_then(|subdivisions| Some(subdivisions.names.as_ref()?.get("en")?.to_string())), | ||
| ), |
There was a problem hiding this comment.
this is failing because subdivisions is a list like this one:
"subdivisions": [
{
"geoname_id": 5332921,
"iso_code": "CA",
"names": {
"de": "Kalifornien",
"en": "California",
"es": "California",
"fr": "Californie",
"ja": "カリフォルニア",
"ru": "Калифорния",
"zh-CN": "加州"
}
}
]
but I'm not too sure how to grab first element in Rust
There was a problem hiding this comment.
something like
city.subdivisions.as_ref().and_then(|subdivisions| {
subdivisions.get(0).and_then(|subdivision| {
subdivision.names.as_ref().and_then(|subdivision_names| {
subdivision_names
.get("en")
.cloned()
.map(|subdivision_name| subdivision_name.to_string())
})
})
});
i think?
i do wonder how often the multi subdivision case comes up and if its worth storing an array of them instead
There was a problem hiding this comment.
it's probably fine just to take the first (top level as i understand it) subdivision.
There was a problem hiding this comment.
based on this example it looks like it's always a list
There was a problem hiding this comment.
I was gonna grab the first element but rust wasn't happy about how I did it. I'll try your suggestion
There was a problem hiding this comment.
also changed it subdivisions -> subdivision
There was a problem hiding this comment.
Looks good!
Some tests are failing, seems like because of the fixtures are not updated yet.
To fix the snapshots for tests, please, see https://github.com/getsentry/relay/blob/master/README.md#snapshot-tests
You will have to install insta as per instruction , run the tests locally add updated snapshots to this PR.
Once it's done we can merge it.
Co-authored-by: Oleksandr <1931331+olksdr@users.noreply.github.com>
|
Can we also make sure we update https://develop.sentry.dev/sdk/event-payloads/user/? |
Now that we store `geo.subdivision` getsentry/relay#2058 make this field queryable Relies on getsentry/snuba#4080 Fixes https://github.com/getsentry/getsentry/issues/10313
expose subdivision field in the search. should be merged after: - [x] getsentry/relay#2058 - [x] getsentry/snuba#4080 - [x] #47995
Store
city.subdivisions(ref) per customer request.Fixes #2057