census-ecosystem / opencensus-go-exporter-aws

OpenCensus Go exporters for AWS (XRay only for now)
Apache License 2.0
27 stars 21 forks source link

Should Close wait for OnExport? #19

Open jcbwlkr opened 6 years ago

jcbwlkr commented 6 years ago

Hi! This exporter is exactly what I needed so thanks for making it! :)

I ran down a bit of a rabbit hole for a while when I ran the example code. I thought it wasn't working because the OnExport function was not printing the trace ID. Eventually I realized that the trace was being sent to X-Ray but the program was closing before the OnExport function could run. Even adding exporter.Close() to the end of func main didn't help in my case.

I see in xray.go line 329 we kick off the func in a goroutine

_, err := e.api.PutTraceSegments(&input)
if err == nil {
    for _, traceID := range traceIDs {
        go e.onExport(OnExport{TraceID: traceID})
    }
    return
}

I figure we're doing that so a slow OnExport func doesn't block the whole process. The problem is what if there is critical code happening in OnExport that might get lost as a server shuts down? I can see a few possibilities

  1. Increment e.wg for each call to e.onExport and wrap it in a closure that calls e.wg.Done. This would make Close block on the OnExports. The problem here might be that a rogue OnExport function could run forever and there'd be no way in this library to shut it down. Maybe that's just FUD though.
  2. Have a separate waitgroup just for these and let people opt in to waiting for it?
  3. Do nothing but document that OnExport is not guaranteed to run. Probably not a great solution either. 🤷‍♂️
savaki commented 6 years ago

Good point. I like the wg suggestion. I think there may be a way to do that and have our cake and eat it too.