Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relaxed sampling for service latency, as 0 disables it. #60

Closed
wants to merge 1 commit into from

Conversation

aricart
Copy link
Member

@aricart aricart commented Dec 3, 2019

Fixed new staticcheck errors

@aricart aricart requested a review from kozlovic December 3, 2019 15:18
@coveralls
Copy link

Pull Request Test Coverage Report for Build 305

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 89.094%

Totals Coverage Status
Change from base Build 299: 0.0%
Covered Lines: 1062
Relevant Lines: 1192

💛 - Coveralls

@@ -60,8 +60,8 @@ type ServiceLatency struct {
}

func (sl *ServiceLatency) Validate(vr *ValidationResults) {
if sl.Sampling < 1 || sl.Sampling > 100 {
vr.AddError("sampling percentage needs to be between 1-100")
if sl.Sampling < 0 || sl.Sampling > 100 {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is correct. We do the same check in the server when parsing local config: https://github.com/nats-io/nats-server/blob/master/server/opts.go#L1932.
When updating an account, if the jwt.Export.Latency (a *jwt.ServiceLatency object) is not nil, then server tries to track for latency and will fail if sampling is < 1 or > 100.
So not sure what is the right thing to do here... @derekcollison wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

we had an informal discussion where we said that 0 would disable the check thus allowing the configuration to have the subject etc, but be turned off easily. Perhaps this is not correct but nsc does allow the zero.

Copy link
Member

Choose a reason for hiding this comment

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

I think its correct, we should remove it all together to disable IMO.

@aricart aricart closed this Dec 3, 2019
@aricart aricart deleted the sampling-validation branch January 17, 2020 19:39
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.

None yet

4 participants