-
Notifications
You must be signed in to change notification settings - Fork 896
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 Sampling Result Names #938
Conversation
- `NOT_RECORD` - `RECORD` - `RECORD_AND_SAMPLED` Seems like we should pick between present tense or past tense and make it consistent. I am proposing that we use past tense as in it's a decision that has already been made. `NOT_RECORDED`, `RECORDED`, `RECORDED_AND_SAMPLED`. `NOT_RECORD` doesn't make sense. It is not correct English.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This was already irking me for quite some time. 😄
I think current description of Samplers in Trace SDK is not very clear. I am looking at Java SDK and there sampling response is kinda imperative: it tells SDK to create a Span with specific behaviour (isRecording, isSampled). So it is not past tense like in "some action has been done". It is more "the decision has been made, now act this way". Therefor I totally agree that names could use some unification, but I think new names should be in imperative mood, not just past tense verbs. |
It is, thanks :)
I disagree, but both ways are Ok. |
Hmm, what would be the imperative names? |
I kind of like them being imperative as well. I think the imperative version would be:
|
What about |
I'm good with that. They are consistent and I think they make sense. |
I now remember there was once confusion about whether IsRecording determines the sampling result or the other way round, see #871 (comment). I think using imperative mood here would actually make this confusion less likely, so I changed my opinion and agree with @iNikem that imperative mood is probably better for this enum. |
@ejsmith if no existing issue filed, can you file one so we can track? |
@@ -93,10 +93,10 @@ Returns the sampling Decision for a `Span` to be created. | |||
It produces an output called `SamplingResult` which contains: | |||
|
|||
* A sampling `Decision`. One of the following enum values: | |||
* `NOT_RECORD` - `IsRecording() == false`, span will not be recorded and all events and attributes | |||
* `IGNORE` - `IsRecording() == false`, span will not be recorded and all events and attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the description says "will be dropped" here, maybe DROP
would be an even better name. I'm fine with IGNORE or SKIP too though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I like DROP
better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for DROP
. This is necessary for opentelemetry-cpp because IGNORE
is defined as macro as constant. This build failure for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ThomsonTan Then the main problem there seems to be that you have defined a macro constant 😃 And isn't the usual C++ idiom to only have macros/defines in ALL_CAPS? Then you would write SamplingResult::Drop
or SamplingResult::drop
or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Oberon00 the problem is not defined by opentelemetry-cpp, as IGNORE
is sounds like a common word, there is chance of name collision. Change casing in C++ sounds a good suggestion though.
* Rename SamplingDecision enum values As prescribed in open-telemetry/opentelemetry-specification#938 and open-telemetry/opentelemetry-specification#956. * Include in Changelog Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
* Rename SamplingDecision enum values As prescribed in open-telemetry/opentelemetry-specification#938 and open-telemetry/opentelemetry-specification#956. * Include in Changelog Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
* Inconsistent SamplingDecision Names - `NOT_RECORD` - `RECORD` - `RECORD_AND_SAMPLED` Seems like we should pick between present tense or past tense and make it consistent. I am proposing that we use past tense as in it's a decision that has already been made. `NOT_RECORDED`, `RECORDED`, `RECORDED_AND_SAMPLED`. `NOT_RECORD` doesn't make sense. It is not correct English. * Use imperative names for sampling result
Current Names
NOT_RECORD
RECORD
RECORD_AND_SAMPLED
Seems like we should pick between present tense or past tense and make it consistent. I am proposing that we use past tense as in it's a decision that has already been made.
NOT_RECORD
doesn't make sense. It is not correct English.Proposed Names
NOT_RECORDED
RECORDED
RECORDED_AND_SAMPLED
Related: open-telemetry/opentelemetry-dotnet#1255