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

Misleading description of IsRecording() in trace API #3006

Closed
utezduyar opened this issue Dec 5, 2022 · 2 comments · Fixed by #3020
Closed

Misleading description of IsRecording() in trace API #3006

utezduyar opened this issue Dec 5, 2022 · 2 comments · Fixed by #3020
Assignees
Labels
area:api Cross language API specification issue editorial Editorial changes only (typos, changelog, ...). No content-related changes of any kind. spec:trace Related to the specification/trace directory

Comments

@utezduyar
Copy link
Contributor

I believe the text below from the spec misleading.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md
IsRecording
Returns true if this Span is recording information like events with the AddEvent operation, attributes using SetAttributes, status with SetStatus, etc.

The text makes me think that if any of these APIs (AddEvent, SetAttribute..) are called, the recording status will be TRUE. Seems like IsRecording returns True if the Span is sampled and the sample hasn't ended. At least the go implementation tells me that.

// IsRecording returns if this span is being recorded. If this span has ended
// this will return false.
func (s *recordingSpan) IsRecording() bool {
	if s == nil {
		return false
	}
	s.mu.Lock()
	defer s.mu.Unlock()

	return s.endTime.IsZero()
}

I believe the current text is valuable but needs to be added in a way that none of these APIs (AddEvent, SetAttribute..) will work (in other terms will be NOOP) after the span is ended. I would be happy to send a patch if I have a point.

@utezduyar utezduyar added the spec:trace Related to the specification/trace directory label Dec 5, 2022
@Oberon00
Copy link
Member

Oberon00 commented Dec 6, 2022

It was meant to say that calling these APIs won't record any information if IsRecording is false, I guess. Wording improvements are welcome.

@Oberon00 Oberon00 added area:api Cross language API specification issue editorial Editorial changes only (typos, changelog, ...). No content-related changes of any kind. labels Dec 6, 2022
utezduyar added a commit to utezduyar/opentelemetry-specification that referenced this issue Dec 7, 2022
@utezduyar
Copy link
Contributor Author

Hi my commit has been waiting for "First-time contributors need a maintainer to approve running workflows". Is this something I need to trigger or please let me know if I need to do anything else.

@arminru arminru linked a pull request Dec 13, 2022 that will close this issue
utezduyar added a commit to utezduyar/opentelemetry-specification that referenced this issue Dec 13, 2022
arminru pushed a commit that referenced this issue Dec 16, 2022
Co-authored-by: Joshua MacDonald <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>
Co-authored-by: Armin Ruech <[email protected]>
Fixes #3006
lmolkova pushed a commit to lmolkova/opentelemetry-specification that referenced this issue Dec 19, 2022
Co-authored-by: Joshua MacDonald <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>
Co-authored-by: Armin Ruech <[email protected]>
Fixes open-telemetry#3006
joaopgrassi pushed a commit to dynatrace-oss-contrib/semantic-conventions that referenced this issue Mar 21, 2024
Co-authored-by: Joshua MacDonald <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>
Co-authored-by: Armin Ruech <[email protected]>
Fixes open-telemetry/opentelemetry-specification#3006
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue editorial Editorial changes only (typos, changelog, ...). No content-related changes of any kind. spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants