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.

Sign up for GitHub

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

Jump to bottom

Update Tracing API: IsRecording wording #3020

Merged
merged 10 commits into from
Dec 16, 2022
Merged

Update Tracing API: IsRecording wording #3020

merged 10 commits into from
Dec 16, 2022

Conversation

utezduyar
Copy link
Contributor

@utezduyar utezduyar commented Dec 7, 2022 •
edited by arminru
Loading

Fixes #3006

Changes

Please provide a brief description of the changes here.

For non-trivial changes, follow the change proposal process and link to the related issue(s) and/or OTEP(s), update the CHANGELOG.md, and also be sure to update spec-compliance-matrix.md if necessary.

Related issues #

Related OTEP(s) #

@utezduyar utezduyar requested review from a team December 7, 2022 07:16
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 7, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: utezduyar / name: Umut Tezduyar Lindskog (de52708)

MrAlias
MrAlias reviewed Dec 9, 2022
View reviewed changes
specification/trace/api.md Outdated Show resolved Hide resolved
@arminru arminru added spec:trace Related to the specification/trace directory area:api Cross language API specification issue labels Dec 13, 2022
@arminru arminru linked an issue Dec 13, 2022 that may be closed by this pull request
Misleading description of IsRecording() in trace API #3006
Closed
arminru
arminru requested changes Dec 13, 2022
View reviewed changes
specification/trace/api.md Outdated Show resolved Hide resolved
@utezduyar
Correct the formulation
0ed0585 
Take in to consideration of non recording spans.
@utezduyar utezduyar requested a review from arminru December 13, 2022 13:04
jmacd
jmacd approved these changes Dec 13, 2022
View reviewed changes
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
yurishkuro
yurishkuro requested changes Dec 13, 2022
View reviewed changes
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
@utezduyar utezduyar requested review from yurishkuro and removed request for arminru December 14, 2022 07:52
@yurishkuro
Copy link
Member

My proposal to change the original lines 457-464:

A span is recording (IsRecording returns true) when the data provided to it via functions like SetAttributes, AddEvent, SetStatus is captured in some form (e.g., in memory). When a span is not recording (IsRecording returns false), all this data is discarded right away, making the span effectively a no-op.

{move paragraph from L474 here}

After a Span is ended, it SHOULD become non-recording and its IsRecording SHOULD start consequently return false. The one known exception to this is streaming implementations of the API that do not keep local state and cannot change the value of IsRecording after ending the span.

@utezduyar
Copy link
Contributor Author

I have mostly used your text, made couple of minor changes.

@utezduyar utezduyar requested review from arminru and yurishkuro and removed request for yurishkuro and arminru December 15, 2022 07:39
arminru
arminru reviewed Dec 15, 2022
View reviewed changes
specification/trace/api.md Outdated Show resolved Hide resolved
yurishkuro
yurishkuro approved these changes Dec 15, 2022
View reviewed changes
specification/trace/api.md Outdated Show resolved Hide resolved
MrAlias
MrAlias reviewed Dec 15, 2022
View reviewed changes
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
@yurishkuro
Copy link
Member

@arminru please re-check if you still require changes

@arminru arminru added the editorial Editorial changes only (typos, changelog, ...). No content-related changes of any kind. label Dec 16, 2022
@arminru arminru merged commit 3f44fd5 into open-telemetry:main Dec 16, 2022
lmolkova pushed a commit to lmolkova/opentelemetry-specification that referenced this pull request Dec 19, 2022
@utezduyar @lmolkova
Update Tracing API: IsRecording wording (open-telemetry#3020)
cc5282d 
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
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@utezduyar
Update Tracing API: IsRecording wording (open-telemetry#3020)
5236f00 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Oberon00 Oberon00 Oberon00 left review comments

@arminru arminru arminru approved these changes

@MrAlias MrAlias MrAlias approved these changes

@jmacd jmacd jmacd approved these changes

@yurishkuro yurishkuro yurishkuro approved these changes

Assignees

@jmacd jmacd

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Misleading description of IsRecording() in trace API
6 participants
@utezduyar @yurishkuro @Oberon00 @jmacd @MrAlias @arminru