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

The name "IsRecordingEvents" misleading as it represent recording of any information, not just Events #44

Closed
SergeyKanzhelev opened this issue May 31, 2019 · 8 comments
Assignees
Labels
needs discussion Need more information before all suitable labels can be applied
Milestone

Comments

@SergeyKanzhelev
Copy link
Member

The semantics of IsRecordingEvents is not only about recording events but also setting attributes.

Should it be called IsActive?

@bogdandrutu
Copy link
Member

Active may be confusing especially with being active in the context. So I would suggest maybe IsRecording?

@SergeyKanzhelev
Copy link
Member Author

I'm good with IsRecording

@spidergears
Copy link

IsAlive, if not alive its dead, dead don't record, they walk though.

@SergeyKanzhelev
Copy link
Member Author

Perhaps we need to clarify semantics of this flag even further. For instance, will name be recorded on span? There may be vendors who need to get the span name and serialize it to the wire. Even when span wasn't recorded. Do we really want to not record attributes?

@SergeyKanzhelev SergeyKanzhelev modified the milestones: API complete, API revision: 07-2019 May 31, 2019
@jmacd
Copy link
Contributor

jmacd commented Jul 1, 2019

This feels low priority, but I would use the most concretely applicable term possible.
It's possible for an implementation that performs tail sampling in process to use a real span that may or may not be recorded. We might call this "IsNoOp" or "IsTraced".

@iredelmeier iredelmeier added the needs discussion Need more information before all suitable labels can be applied label Jul 30, 2019
@SergeyKanzhelev SergeyKanzhelev modified the milestones: API revision: 07-2019, Alpha v0.2 Sep 27, 2019
@SergeyKanzhelev SergeyKanzhelev self-assigned this Oct 3, 2019
@SergeyKanzhelev SergeyKanzhelev changed the title API: Is the name "IsRecordingEvents" OK or needs to be changed to "IsActive" The name "IsRecordingEvents" misleading as it represent recording of any information, not just Events Oct 3, 2019
@SergeyKanzhelev
Copy link
Member Author

I changed title to explain where the confusion can be from. IsRecordingEvents responsible for more than just events recording. So I'd suggest to change the name to IsRecording as a minimal first step. Happy to discuss further for alternative names.

@tedsuo
Copy link
Contributor

tedsuo commented Oct 4, 2019

I'm fine with IsRecording, though perhaps IsEnabled might avoid some confusion for cases like tail-based sampling, where you don't technically know if the span will be recorded or not.

@SergeyKanzhelev
Copy link
Member Author

IsRecording is used to record something even if it was sampled out. In case of tail based sampling I'd think sampled flag will be true anyway and will set recording to true automatically.

SergeyKanzhelev pushed a commit to SergeyKanzhelev/opentelemetry-specification that referenced this issue Oct 7, 2019
SergeyKanzhelev added a commit to SergeyKanzhelev/opentelemetry-specification that referenced this issue Feb 18, 2020
TuckTuckFloof pushed a commit to TuckTuckFloof/opentelemetry-specification that referenced this issue Oct 15, 2020
* Start Recorder api

* Fill out Span api

* Reformat

* Fill out Tracer class

* Add cmake build

* Add commenting

* Fix formatting

* Reformat

* Fix date

* Make mutex mutable

* s/mutex_/mu_/

* Remove AddEvent with steady timestamp

* Fix typo

* Fill in IsRecordable

* Use namespace macros

* Add thread-compatible note

* Add commenting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Need more information before all suitable labels can be applied
Projects
None yet
Development

No branches or pull requests

6 participants