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(general): Indicate if OS-version may be frozen #1945

Merged
merged 9 commits into from
Mar 17, 2023

Conversation

TBS1996
Copy link
Contributor

@TBS1996 TBS1996 commented Mar 16, 2023

Implements the following Sentry-PR: getsentry/sentry#45369

Figured relay would be a better fit, a benefit to implementing it in relay is that the OS-version is only potentially frozen if it's from the user-agent, not from the client hints, which is easier to figure out here.

@@ -191,7 +210,7 @@ OsContext {
let headers = vec![
Annotated::new((
Annotated::new("user-agent".to_string().into()),
Annotated::new(r#"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/109.0.0.0 Safari/537.36"#.to_string().into()),
Annotated::new(r#"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/109.0.0.0 Safari/537.36"#.to_string().into()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I wanted to test this functionality in a separate test, I changed the version here so that it wouldn't trigger the '>=' prefix

@TBS1996 TBS1996 marked this pull request as ready for review March 16, 2023 13:40
@TBS1996 TBS1996 requested a review from a team March 16, 2023 13:40
@AbhiPrasad
Copy link
Member

Before we decide to take this option, what about the option of adding extra metadata that we can render in the UI?

So if we detect that an os-version may be frozen, we send additional info down with the event that the UI/product can use to indicate this (maybe even tell people about client hints).

@TBS1996
Copy link
Contributor Author

TBS1996 commented Mar 16, 2023

Before we decide to take this option, what about the option of adding extra metadata that we can render in the UI?

So if we detect that an os-version may be frozen, we send additional info down with the event that the UI/product can use to indicate this (maybe even tell people about client hints).

like a tooltip info giving some more info sounds good to me, although I don't think our users can do much about it. If the end-user doesn't have client hints they would have to switch to a browser that supports it.

would it be too primitive to simply do an equality check in the UI? if version == ">=10" then show tooltip?

@AbhiPrasad
Copy link
Member

would it be too primitive to simply do an equality check in the UI? if version == ">=10" then show tooltip?

I think this makes a lot of sense to me. We should probably then create a GH issue to track this after this gets merged in!

Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

Looks good to me! Please also add a unit test for the Windows case.

@TBS1996 TBS1996 requested a review from jjbayer March 17, 2023 09:13
Copy link
Contributor

@iker-barriocanal iker-barriocanal left a comment

Choose a reason for hiding this comment

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

🚀

@TBS1996 TBS1996 merged commit 0d52aad into master Mar 17, 2023
@TBS1996 TBS1996 deleted the feat/indicatefrozen branch March 17, 2023 12:07
jan-auer added a commit that referenced this pull request Mar 20, 2023
* master:
  ref(project): Inject services into  the `Project` (#1928)
  feat(general): Indicate if OS-version may be frozen (#1945)
  ref(server): Move Envelope into EnvelopeContext (#1936)
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