Skip to content

Revert "remove default rate limit in priority sampler (#796)"#812

Merged
brettlangdon merged 1 commit into
masterfrom
brettlangdon/add.default.rate.limit
Dec 30, 2019
Merged

Revert "remove default rate limit in priority sampler (#796)"#812
brettlangdon merged 1 commit into
masterfrom
brettlangdon/add.default.rate.limit

Conversation

@brettlangdon

Copy link
Copy Markdown
Member

This reverts commit 7ab6af0.

What does this PR do?

Adds back the default 100 traces per second rate limit as a safety net for high volume customers.

@brettlangdon
brettlangdon requested a review from a team as a code owner December 27, 2019 18:03

@labbati labbati 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.

👍 LGTM!

@brettlangdon
brettlangdon merged commit fbd1dc0 into master Dec 30, 2019
@brettlangdon
brettlangdon deleted the brettlangdon/add.default.rate.limit branch December 30, 2019 16:13
@rochdev

rochdev commented Jan 7, 2020

Copy link
Copy Markdown
Member

The default rate limit was originally removed since it provides no advantage and introduces several issues:

  • Unexpected behavior, as the expectation of setting a sample rate of 1.0 is to get 100% of traces.
  • False sense of safety since there could still be billions of traces per month even with the rate limit.
  • Law of least astonishment is usually to have the simplest default, and let the user add more complex configuration.
  • It doesn't help the user in any way to know if their cost is spiking.

Overall it results in a poor UX for no reason. We should make sure to remove it before going GA.

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