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: add geolocation to trace events #724

Merged
merged 6 commits into from
Sep 14, 2023
Merged

feat: add geolocation to trace events #724

merged 6 commits into from
Sep 14, 2023

Conversation

alnr
Copy link
Contributor

@alnr alnr commented Sep 13, 2023

No description provided.

Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

There is even more headers that might come in handy, e.g. cf-region. Do we really want to make this a slice? I think that will make it quite hard to use for queries, e.g. query by country. What if (for whatever reason) only the cf-city header is set but not cf-country? Or the other way around? What if we want to add the region later?

@alnr
Copy link
Contributor Author

alnr commented Sep 13, 2023

There is even more headers that might come in handy, e.g. cf-region. Do we really want to make this a slice? I think that will make it quite hard to use for queries, e.g. query by country. What if (for whatever reason) only the cf-city header is set but not cf-country? Or the other way around? What if we want to add the region later?

Copypasta from https://github.com/ory/kratos/blob/159c13142a11d4a9cd006da5d0afdda9e0f94ca9/session/session.go#L272-L278

We could factor our this functionality and put it in ory/x. 🤷

@zepatrik
Copy link
Member

I still think it makes more sense to put city and country in different attributes. For sessions, it is fine to produce a human-readable string instead, as the information is only a hint for users to distinguish sessions. In event attributes however, we want to query by the attribute, which in turn is hard when multiple attributes are mixed into one slice.

@alnr
Copy link
Contributor Author

alnr commented Sep 13, 2023

I don't disagree. Up to @mszekiel. I just wanted to show him where to add this data.

@mszekiel
Copy link
Contributor

Okay, I'll take this PR with me as Arne mentioned that's an intro and will continue changing the data structure.

@mszekiel mszekiel requested review from mszekiel and removed request for mszekiel September 14, 2023 08:17
Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

Just a few minor improvements and FYIs.

otelx/semconv/events.go Outdated Show resolved Hide resolved
httpx/client_info.go Outdated Show resolved Hide resolved
otelx/semconv/context.go Outdated Show resolved Hide resolved
@zepatrik zepatrik merged commit 78da4d7 into master Sep 14, 2023
6 checks passed
@zepatrik zepatrik deleted the geolocation-events branch September 14, 2023 11:19
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.

4 participants