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

Inconsistent SamplingDecision Names #1255

Closed
ejsmith opened this issue Sep 10, 2020 · 4 comments · Fixed by #1297
Closed

Inconsistent SamplingDecision Names #1255

ejsmith opened this issue Sep 10, 2020 · 4 comments · Fixed by #1297
Labels
bug Something isn't working

Comments

@ejsmith
Copy link
Contributor

ejsmith commented Sep 10, 2020

  • NotRecord
  • Record
  • RecordAndSampled

Seems like we should pick between present tense or past tense and make it consistent. Either Recorded / RecordedAndSampled or Record / RecordAndSample.

NotRecord doesn't make sense. It's not correct English. Maybe just Skip or DontRecord. Personally I like just Skip better. It's already in an enum so it's always going to be qualified like SamplingDecision.Skip.

My vote:

  • SamplingDecision.Skip
  • SamplingDecision.Record
  • SamplingDecision.RecordAndSample

https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry/Trace/SamplingDecision.cs

@ejsmith ejsmith added the bug Something isn't working label Sep 10, 2020
@cijothomas
Copy link
Member

The names were borrowed from the spec.
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#sampler
Agree its not consistent.

We should try to change the spec naming as well.

@ejsmith
Copy link
Contributor Author

ejsmith commented Sep 10, 2020

Ahh, yeah, I was thinking that was probably the case. Seems unlikely that we would get the spec to change, right? Consistency across the implementations/spec is much more important than the individual names.

@cijothomas
Copy link
Member

Small renamings are likely not controversial and you can try submitting a PR to the spec repo :)

@cijothomas
Copy link
Member

This is done. Thank @ejsmith for making the fix in the spec itself :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants