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

HTTP transporter does not send open records when node process is about to terminate #33

Closed
ocastx opened this issue Aug 17, 2016 · 9 comments

Comments

@ocastx
Copy link

ocastx commented Aug 17, 2016

The current implementation of the HTTP transporter does not send open records in it's queue when the node process finished executing (no events left in the queue).

if (timer.unref) { // unref might not be available in browsers
     timer.unref(); // Allows Node to terminate instead of blocking on timer
}

Why shouldn't the transporter send all open records before exiting?

@jquatier
Copy link
Contributor

jquatier commented Aug 17, 2016

The reason we unref the timer is so the process exits when there's nothing else running in the event loop, otherwise your process would continue running forever. There's a process.on('exit') hook that could be used, but you can only do synchronous operations there, so that wouldn't work.

Anyway, would like to hear if anyone has a good solution. I do remember @eirslett telling me that high consistency isn't a requirement for Zipkin trace data, so I think that's why we might assume that if a few spans are lost as the server is coming down, it's an acceptable scenario. The problem of course is if your Node script just runs for a second or a half-second from start to finish, you'll never get anything unless you wait for the recorder to run. You can set the httpInterval to something much smaller than a second if needed.

@ocastx
Copy link
Author

ocastx commented Aug 17, 2016

I would suggest adding a setTimeout with the same httpInterval which is overwritten every time a new span is put into the queue. With this the node process would wait the minimum time so the interval fires. Also if the queues is empty and the timeout is still running it could then be terminated so the process would not shutdown gracefully with a delay of like one sec.

I already built this to fix my problem, could create a PR for this if wanted.

@jquatier
Copy link
Contributor

That sounds like a good solution. I think a similar change could be made to the BatchRecorder since it also uses a timer. Hopefully @sveisvei or @adriancole can comment on this, too.

@jthomas
Copy link

jthomas commented Jan 21, 2017

I also hit this issue during development of a new instrumentation for a serverless function platform.

Serverless platforms spawn a new process for each invocation. Once the function handler returns, the process is terminated. This means the final spans often haven't finished sending before the process exits.

I worked around this issue but would prefer a custom event or configuration flag to modify this behaviour in the HTTP logger.

@codefromthecrypt
Copy link
Member

most instrumentation I've noticed attempt to flush on close, although not always via a shutdown hook. For example, in the java side, up to 1 second of blocking is permitted before spans are dropped on close

@codefromthecrypt
Copy link
Member

@jquatier curious about the statement about not being able to do synchronous operations.. in a shutdown scenario, would synchronous flush (with timeout) be a problem ? (I understand why it would be in a non-shutdown one).

@codefromthecrypt
Copy link
Member

added "help-wanted" as at least 3 libraries are hacking around this, and at least to me, that suggests we could either document or support a pattern for this.

@jcchavezs
Copy link
Contributor

jcchavezs commented Jul 21, 2019

Will this issue be solved by #417 or could we do something else using the flush method? cc @adriancole

@jcchavezs
Copy link
Contributor

I will close this issue as I believe #417 fixes this issue. Feel free to raise any further concern @ocastx.

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

Successfully merging a pull request may close this issue.

5 participants