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

Should ending a span force its children to end by default? #4

Closed
bg451 opened this issue May 20, 2019 · 6 comments
Closed

Should ending a span force its children to end by default? #4

bg451 opened this issue May 20, 2019 · 6 comments

Comments

@bg451
Copy link
Member

bg451 commented May 20, 2019

Howdy y'all, I've been combing through the opencensus-node code in preparation of the node merger happening.

In opencensus-node, there's this interesting bit of code in (Span).end that forces all children spans to end when the parent span ends. I don't think this should be default behavior as I've seen many valid situations where the parent ends before the child, particularly in follows from situations where you have async processes. I do like the debug statement being there, and I like the idea of adding an option to force child spans to end.

Should child span truncation continue to be default behavior?
For opencensus folk, what was the rationale behind it?

@mayurkale22
Copy link
Member

This looks like a legacy implementation and I am not sure about the logic. I think we should not close child spans forcefully (by default), and like the idea of passing an option to force child spans to end.

Few questions floated in my mind,

  • Does it cause memory leakage if anyone failed to end the span, due to bad code?
  • How does it get visualized in Zipkin, Jaeger etc exporters? Does it create separate orphan span?

@SergeyKanzhelev
Copy link
Member

it is a good point that cross languages spec needs to document this decision. As an experiment, I've added spec-needed label and corresponding issue: open-telemetry/opentelemetry-specification#18

@rochdev
Copy link
Member

rochdev commented May 23, 2019

There are tons of valid use cases for a child span to finish after its parent, and in some cases even start after its parent is finished. This is especially true in Node where everything happens asynchronously.

What is the use case for the configuration option? When would you want to auto finish the children?

@hekike
Copy link
Member

hekike commented May 23, 2019

+1 on not closing child spans automatically. For the same reason, I find it hard to get span ending casualty right around closing an operation.

@tedsuo
Copy link

tedsuo commented May 30, 2019

Sounds like there is agreement that we should not close the child spans (I agree as well).

Also, I feel this is a spec (cross-language) issue, and it should work the same in all languages.

@mayurkale22
Copy link
Member

Annosha added a commit to Annosha/opentelemetry-js that referenced this issue Oct 20, 2024
# This is the 1st commit message:

Added local test pkg

# This is the commit message #2:

add type declaration for test-none-core-module

# This is the commit message #3:

Replaced cpx2 with a local module

# This is the commit message open-telemetry#4:

fixed lint errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants