Skip to content

handle sample rate type incompatibilities#2036

Merged
st0012 merged 8 commits intogetsentry:masterfrom
chronograph-pe:nb-sample-rate-cast-to-float
Jun 18, 2023
Merged

handle sample rate type incompatibilities#2036
st0012 merged 8 commits intogetsentry:masterfrom
chronograph-pe:nb-sample-rate-cast-to-float

Conversation

@nbr
Copy link
Copy Markdown
Contributor

@nbr nbr commented Apr 25, 2023 •
edited
Loading

Description

The suggested workaround in #2035 results in an ArgumentError, since we aren't casting to Float. This PR will allow us to fail earlier and with a more specific error message.

@nbr nbr force-pushed the nb-sample-rate-cast-to-float branch 2 times, most recently from fdeca99 to 00bfdf1 Compare April 25, 2023 19:43
@nbr nbr marked this pull request as ready for review April 25, 2023 19:47
st0012
st0012 reviewed Apr 30, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

Thanks for raising the issue. I think in the case of configurations, we should raise errors to explicitly warn users about unexpected values.

@github-actions
Copy link
Copy Markdown

github-actions bot commented May 22, 2023

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@nbr nbr changed the title gracefully handle sample rate type incompatibilities handle sample rate type incompatibilities May 27, 2023
@nbr nbr requested a review from st0012 May 27, 2023 15:52
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 2, 2023 •
edited
Loading

Codecov Report

Patch coverage: 83.33% and project coverage change: +0.02 🎉

Comparison is base (a231150) 83.19% compared to head (0d99b2f) 83.21%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2036      +/-   ##
==========================================
+ Coverage   83.19%   83.21%   +0.02%     
==========================================
  Files         119      119              
  Lines        5653     5661       +8     
==========================================
+ Hits         4703     4711       +8     
  Misses        950      950
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/configuration.rb 83.47% <83.33%> (+0.14%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@st0012 st0012 added this to the 5.10.0 milestone Jun 2, 2023
st0012
st0012 reviewed Jun 2, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

This will result in Configuration#trace_sample_rate == 0.0.

I was a bit confused by this statement. Is that still true after your change?

@nbr
Copy link
Copy Markdown
Contributor Author

nbr commented Jun 5, 2023

This will result in Configuration#trace_sample_rate == 0.0.

I was a bit confused by this statement. Is that still true after your change?

It is no longer true, I've now updated the description.

@nbr nbr requested a review from st0012 June 5, 2023 18:48
@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Jun 7, 2023

This looks good now. Can you add a changelog entry for it? Thx

@st0012 st0012 mentioned this pull request Jun 7, 2023
Merged
@nbr nbr force-pushed the nb-sample-rate-cast-to-float branch from 2aaf83c to 0d99b2f Compare June 12, 2023 18:06
@nbr
Copy link
Copy Markdown
Contributor Author

nbr commented Jun 12, 2023

@st0012 Changelog entry added here: 0d99b2f. Thanks for your time reviewing this.

st0012
st0012 approved these changes Jun 18, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

Thank you so much!

@st0012 st0012 merged commit 11ecd25 into getsentry:master Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@st0012 st0012 st0012 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

sentry-ruby

Projects

None yet

Milestone

5.10.0

Development

Successfully merging this pull request may close these issues.

2 participants

@nbr @st0012