Skip to content

feat(sampling): Add specify client modal - [TET-279]#37251

Merged
priscilawebdev merged 11 commits into
masterfrom
feat/sampling-add-specificy-current-client-sample-rate
Aug 2, 2022
Merged

feat(sampling): Add specify client modal - [TET-279]#37251
priscilawebdev merged 11 commits into
masterfrom
feat/sampling-add-specificy-current-client-sample-rate

Conversation

@priscilawebdev

@priscilawebdev priscilawebdev commented Jul 29, 2022

Copy link
Copy Markdown
Member

What this PR does:

  • Add the new modal "specify client rate modal"
  • Update chart logic in the Uniform rate Modal to display discarded buckets according to the specified client rate

What will be done in a follow-up:

  • Add new sampling analytic code to reload
  • Add new sampling analytic code to analytics
  • Add Tests

Preview:

image

@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jul 29, 2022
@priscilawebdev priscilawebdev marked this pull request as ready for review July 29, 2022 13:51
@priscilawebdev priscilawebdev requested a review from a team as a code owner July 29, 2022 13:51
@Jesse-Box

Copy link
Copy Markdown
Contributor

@priscilawebdev I noticed, some line-height and spacing inconsistencies between this modal and the next one.

Can you use the below modal as the point of truth for adjusting the line-height and spacing.
Screen Shot 2022-08-01 at 10 36 04

@priscilawebdev

Copy link
Copy Markdown
Member Author

@Jesse-Box good catch... that's because I defined it as a label of the input and not a TextBlock... will update it

@priscilawebdev priscilawebdev changed the title feat(sampling): Add specify client modal - [TET - 279] feat(sampling): Add specify client modal - [TET-279] Aug 1, 2022

@matejminar matejminar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you double-check this pls?

Kapture.2022-08-01.at.15.50.33.mp4

Comment thread static/app/utils/analytics/samplingAnalyticsEvents.tsx
Comment thread static/app/utils/analytics/samplingAnalyticsEvents.tsx
@priscilawebdev

Copy link
Copy Markdown
Member Author

Can you double-check this pls?

thank you! Issue shall be fixed now... good catch

@priscilawebdev

priscilawebdev commented Aug 1, 2022

Copy link
Copy Markdown
Member Author

BTW when they are editing the uniform rule and discard is not present in the outcome data... shall we display the "0" step? maybe we can discuss this offline

Comment on lines +277 to +293
if (activeStep === Step.SET_CURRENT_CLIENT_SAMPLE_RATE) {
return (
<SpecifyClientRateModal
{...props}
Header={Header}
Body={Body}
Footer={Footer}
closeModal={closeModal}
onReadDocs={onReadDocs}
organization={organization}
projectId={project.id}
onGoNext={value => {
setSpecifiedClientRate(Number(value));
setActiveStep(Step.SET_UNIFORM_SAMPLE_RATE);
}}
/>
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've noticed one more bug.
If we hit back on the uniform rate modal the client rate is lost.
How about we keep it in a state of the uniformRateModal and pass it as props to the SpecifyClientRateModal (and updater function too)? We could remove the useState from the SpecifyClientRateModal and the value would be remembered when going back and forth between modals.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

good catch...thanks

@matejminar matejminar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me!

@priscilawebdev priscilawebdev enabled auto-merge (squash) August 2, 2022 11:44
@priscilawebdev priscilawebdev merged commit 8bf50a8 into master Aug 2, 2022
@priscilawebdev priscilawebdev deleted the feat/sampling-add-specificy-current-client-sample-rate branch August 2, 2022 11:45
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 17, 2022
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.

3 participants