Skip to content

Conversation

@ArthurKnaus
Copy link
Member

Add a form for setting the target sample rate.

Screen.Recording.2024-10-18.at.09.50.27.mov

Closes https://github.com/getsentry/projects/issues/206

@ArthurKnaus ArthurKnaus requested a review from a team October 18, 2024 07:53
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Oct 18, 2024
@shellmayr
Copy link
Member

shellmayr commented Oct 18, 2024

The demo video shows both a period and a comma as the decimal separator in the input and in the feedback below - can we unify this somehow?

Copy link
Member

@obostjancic obostjancic left a comment

Choose a reason for hiding this comment

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

🚀

return api.requestPromise(endpoint, {
method: 'PUT',
data: {
targetSampleRate: Number(fields.targetSampleRate.value) / 100,
Copy link
Member

Choose a reason for hiding this comment

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

just a thought: do we want to make sure this is of certain precision?

@ArthurKnaus
Copy link
Member Author

The demo video shows both a period and a comma as the decimal separator in the input and in the feedback below - can we unify this somehow?

@shellmayr yes, I noticed that too. Out of whatever reason the native number input insists on using , in chromium based browsers. In Firefox it behaves correctly. Implementing a custom number input based on a text field is quite some effort, so I did not want to do this in this PR. Also I would like to see if it behaves like this for others too as it might be tied to specific language settings.

@shellmayr
Copy link
Member

The demo video shows both a period and a comma as the decimal separator in the input and in the feedback below - can we unify this somehow?

@shellmayr yes, I noticed that too. Out of whatever reason the native number input insists on using , in chromium based browsers. In Firefox it behaves correctly. Implementing a custom number input based on a text field is quite some effort, so I did not want to do this in this PR. Also I would like to see if it behaves like this for others too as it might be tied to specific language settings.

Ah ok, gotcha - thanks! I think we should definitely take care of this before launching it widely, but I agree let's see what the conditions are under which this happens so we can make it work thoroughly.

Comment on lines +102 to +104
<Button disabled={!formState.hasChanged || isPending} onClick={handleReset}>
{t('Reset')}
</Button>
Copy link
Member

Choose a reason for hiding this comment

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

should we add a tooltip informing the users why the button is disabled? It is pretty self-explanatory to me but imo it would be nice to have.

@priscilawebdev
Copy link
Member

The 'Switch to Setup' button is overflowing the container on some viewports. Could we look into that?

image

@ArthurKnaus
Copy link
Member Author

Thx for the detailed reviews! As the whole view is very much a WIP, I collected the non code feedback in a ticket and will revisit it once the rest of the UI is implemented.

@codecov
Copy link

codecov bot commented Oct 21, 2024

Bundle Report

Changes will increase total bundle size by 62.52kB (0.2%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
app-webpack-bundle-array-push 30.97MB 62.52kB (0.2%) ⬆️

@codecov
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

Attention: Patch coverage is 0% with 86 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...app/views/settings/dynamicSampling/formContext.tsx 0.00% 41 Missing ⚠️
...views/settings/dynamicSampling/dynamicSampling.tsx 0.00% 24 Missing ⚠️
...settings/dynamicSampling/targetSampleRateField.tsx 0.00% 11 Missing ⚠️
...s/settings/dynamicSampling/dynamicSamplingForm.tsx 0.00% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #79341      +/-   ##
==========================================
- Coverage   78.34%   78.31%   -0.03%     
==========================================
  Files        7123     7136      +13     
  Lines      314824   315162     +338     
  Branches    51460    51506      +46     
==========================================
+ Hits       246639   246818     +179     
- Misses      61727    61880     +153     
- Partials     6458     6464       +6     

@ArthurKnaus ArthurKnaus merged commit 1be916d into master Oct 21, 2024
43 of 44 checks passed
@ArthurKnaus ArthurKnaus deleted the aknaus/feat/dynamic-sampling/settings-for-sample-rate branch October 21, 2024 07:27
jan-auer added a commit that referenced this pull request Oct 21, 2024
* master: (288 commits)
  feat(metrics): Register MRI for spans/count_per_root_project (#78992)
  feat(dynamic-sampling): Settings for sample rate (#79341)
  Revert "feat(sentry-sdk): Enable HTTP2 transport" (#79391)
  fix(feedback): keep oldest date_added for duplicate user reports (#79387)
  chore(issue-stream): Remove tooltip for Unhandled (#79385)
  chore(autofix): Show banner if gen AI consent is given, even if no feature flag (#79362)
  chore(autofix+copilot) Allow autofix without FF if gen AI consent given (#79361)
  Fixes VULN-50 by enforcing option (#79384)
  perf(issues): improve adjacent_events query (#79365)
  feat(issues): Add anchor links back to issue sections (#79333)
  fix(issue-views): Make tab bar take up entire row (#79383)
  chore(issues): Add additional metrics for ownership matching (#79302)
  feat(insights): create screen rendering module (#79192)
  fix(issues): Avoid streamline issue layout rerenders (#79327)
  ref(performance): Add missing types to performance widgets (#79301)
  chore(issue-views): Add translation wrapper to aria label (#79320)
  chore(issue-stream): Reduce font size of title and message (#79378)
  feat(insights): update headers and breadcrumbs on frontend domain view (#78945)
  feat(insights): add view trends button to ai overview (#78611)
  ref(rr6): Remove unused param (#79379)
  ...
kneeyo1 pushed a commit that referenced this pull request Oct 21, 2024
cmanallen pushed a commit that referenced this pull request Oct 23, 2024
Add a form for setting the target sample rate.

Closes getsentry/projects#206
@github-actions github-actions bot locked and limited conversation to collaborators Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants