-
Notifications
You must be signed in to change notification settings - Fork 887
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
Clarify behavior of Span.End, interaction with IsRecording #1011
Clarify behavior of Span.End, interaction with IsRecording #1011
Conversation
Overall good, thanks for adding this, LGTM. |
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
@tsloughter Since you commented on this PR quite a bit, it would be great if you could "formally" review it, and ideally approve if you think the change is good. |
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.
So I think I'm ok with this because its in the API. My main concern at this point was "MAY leak memory" and "responsibility of the user".
I was going to suggest including something about how implementations can optionally include cleanup code to handle leak cases, but I guess that belongs in the SDK?
In the Erlang SDK we have 2 ways to deal with un-End'ed Spans. One is the "sweeper", this is optionally enabled and configured to look for Spans that have been started and not ended for a configurable amount of time, it can then mark them with an attribute like "ended by sweeper" and End them.
The other is an optional process that monitors processes with started Spans and each Span stores the PID of the process it was started in -- still haven't merged this one as I haven't fixed it to work with Spans started in one process and attached in another -- and if the process crashes the monitor handles cleanup of all the Spans in that process, End'ing them and marking them in some way as having crashed.
Since this is SDK stuff I'm not sure anything needs to be put in the API spec about it.
I think it may make sense to add at least the first one to the SDK spec as an optionally available span processor. Also, in Java there is an issue (open-telemetry/opentelemetry-java#1084) about a SpanProcessor that We could consider adding something to the SDK spec that the default SDK "SHOULD NOT" leak resources even for un-ended spans if it's possible (e.g. in most garbage collected languages, this should be the natural default; only if you grab a (strong) reference in OnStart, then you may cause a leak). However, I think all this belongs in a follow-up issue. Do you want to create one? EDIT: And thank you for the review! 😊 |
@Oberon00 yea, I can create another issue or a PR to track that. |
@open-telemetry/specs-approvers Feel free to review this PR. We reached agreement on a missing detail regarding recording after |
@carlosalberto or @open-telemetry/technical-committee: I think we can merge this now. |
Oh, wait a minute, I just got reminded by @arminru to add an entry to the compliance matrix for the IsRecording change. Adding it now... |
Updated compliance matrix & build is green. So now this should really be ready to merge 😃 @carlosalberto |
Fixes #1010
Changes