sentry icon indicating copy to clipboard operation
sentry copied to clipboard

feat(dynamic-sampling): Settings for sample rate

Open ArthurKnaus opened this issue 1 year ago • 4 comments

Add a form for setting the target sample rate.

https://github.com/user-attachments/assets/fff166c4-924d-4c4a-8cc9-e20a78c0ef62

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

ArthurKnaus avatar Oct 18 '24 07:10 ArthurKnaus

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 avatar Oct 18 '24 08:10 shellmayr

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.

ArthurKnaus avatar Oct 18 '24 09:10 ArthurKnaus

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.

shellmayr avatar Oct 18 '24 09:10 shellmayr

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

image

priscilawebdev avatar Oct 18 '24 09:10 priscilawebdev

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.

ArthurKnaus avatar Oct 21 '24 06:10 ArthurKnaus

Bundle Report

Changes will increase total bundle size by 62.52kB (0.2%) :arrow_up:. This is within the configured threshold :white_check_mark:

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

codecov[bot] avatar Oct 21 '24 07:10 codecov[bot]

Codecov Report

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

:white_check_mark: All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...app/views/settings/dynamicSampling/formContext.tsx 0.00% 41 Missing :warning:
...views/settings/dynamicSampling/dynamicSampling.tsx 0.00% 24 Missing :warning:
...settings/dynamicSampling/targetSampleRateField.tsx 0.00% 11 Missing :warning:
...s/settings/dynamicSampling/dynamicSamplingForm.tsx 0.00% 10 Missing :warning:
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     

codecov[bot] avatar Oct 21 '24 07:10 codecov[bot]