stats/view: add Flush to immediately report all collected data points regardless of buffering #862

lychung83 · 2018-08-09T00:29:41Z

Problem:

When exiting a program running opencensus monitoring, some of the data are not passed to exporter. This happens because as one can see in following code, reportUsage() is called only by timer, so when program exists, all data passed to opencensus after last timer firing event are simply lost.

func (w *worker) start() {
	for {
		select {
		case cmd := <-w.c:
			cmd.handleCommand(w)
		case <-w.timer.C:
			w.reportUsage(time.Now())
		case <-w.quit:
			w.timer.Stop()
			close(w.c)
			w.done <- true
			return
		}
	}
}

Solution proposal:

Make func (w *worker) stop() to be (indirectly) callable by client of package.
Make func (w *worker) start() to call w.reportUsage() before returning when <-w.quit is triggered.

The text was updated successfully, but these errors were encountered:

odeke-em · 2018-09-04T19:19:27Z

Thank you for filing this issue @lychung83!

So currently, (*worker).stop() is NOT called by any function nor is case <-w.quit triggered by any exit condition. The app just exits if exiting.

I think the idiomatic fix here would to add a global function to view called say End() which will invoke (*worker)(defaultWorker).stop and that should involve passing w.reportUsage as you've suggested. However, that's going to cause a send on closed channel panic with the current stateful design of the worker.

/cc @rakyll @Ramonza

rakyll · 2018-09-04T19:34:18Z

I think we need a global Flush function that reports all the collected points.

odeke-em · 2018-09-04T19:41:13Z

@rakyll perhaps I am bikeshedding or not, on the name: I thought of Flush too, but that name implies that continued usage after its invocation is allowed, yet we want a function that is terminally invoked, hence the recommendation of End :)

rakyll · 2018-09-04T19:44:27Z

Flush function I was suggesting is not stopping the worker, it only flushes the unprocessed data points. Exporting a function that stops the worker might need more thinking because we never planned to give control of the worker to the user entirely.

Also, in practice, should you care about what's collected after you flushed due to the termination signal?

odeke-em · 2018-09-04T20:01:01Z

Flush function I was suggesting is not stopping the worker, it only flushes the unprocessed data points.

Roger that, that would work. And in deed, users can invoke defer view.Flush() before their main exits so it kills 2 birds with 1 stone.

Also, in practice, should you care about what's collected after you flushed due to the termination signal?

@lychung83 is this an actual problem that you've encountered or were you hypothesizing based off reading the code? I too don't think it should matter, and moreoever we haven't even provided a stop method nor does the worker know when we are terminating except by the Go runtime cleaning up.

lychung83 · 2018-09-05T00:30:39Z

To me, Flush() (not stopping the worker) seems enough. My use cases are:

used at the end of the program to report any remaining data points in the worker.
used for the test program to make sure all data points put by any measure.M() call to the exporter inspected by test w/o calling time.Sleep() for certain amount of time that's enough to fire worker.c

odeke-em · 2018-09-20T22:14:35Z

Okay, am retitling this issue then as Flush is request that is different from the original request of when a program is exiting.

Flush is a global function that immediately reports all collected data by the worker, regardless of the reporting duration or buffering. Added tests but also ensured that calling (*worker).stop() then Flush doesn't block and returns ASAP if the state is "quit". The required protected w.quit from double close but also using a select to detect cases of forever waiting channel `w.quit` which was closed. Fixes census-instrumentation#862

razvanh · 2018-09-21T18:51:53Z

This will solve census-ecosystem/opencensus-go-exporter-stackdriver#43.

Fixes census-instrumentation#862.

semistrict · 2018-09-25T19:14:33Z

I don't think we should have a separate function to flush each Exporter and one to flush the internal stats buffer. We should have a single global Flush/Shutdown/Close function that only returns when everything has been flushed. It's also important that things get flushed in the right order: flushing the internal stats before flushing the buffer in the exporter will not work as intended. Users should not have to find this out themselves.

Instrument interceptor/opencensus with stats to record the number of received spans. This is accomplished by a helper to process spans that sends over spans even if the number of spans is 0. Also record with tag_key "opencensus_interceptor" whose value is the name of the respective interceptor. The test to ensure that 0 length spans are also added is currently disabled because: * Issue census-instrumentation/opencensus-go#862 is not yet implemented which requests that the OpenCensus-Go stats worker provide a method Flush to flush all data. Without it, metrics will contain stale data from previous tests. Add tests to lock-in this behavior. Updates #63

Instrument interceptor/opencensus with stats to record the number of received spans. This is accomplished by a helper to process spans that sends over spans even if the number of spans is 0. Also record with tag_key "opencensus_interceptor" whose value is the name of the respective interceptor. The test to ensure that 0 length spans are also added is currently disabled because: * Issue census-instrumentation/opencensus-go#862 is not yet implemented which requests that the OpenCensus-Go stats worker provide a method Flush to flush all data. Without it, metrics will contain stale data from previous tests. Add tests to lock-in this behavior. Also replace grpc.NewServer with a stats enabled server helper call. This enables tracing and metrics for each gRPC server by replacing naked usages of grpc.NewServer with the new helper function: internal.GRPCServerWithObservabilityEnabled Updates #63

Flush is a global function that immediately reports all collected data by the worker, regardless of the reporting duration or buffering. Added tests but also ensured that calling (*worker).stop() then Flush doesn't block and returns ASAP if the state is "quit". The required protected w.quit from double close but also using a select to detect cases of forever waiting channel `w.quit` which was closed. Fixes census-instrumentation#862

rghetia · 2019-03-27T05:11:35Z

For metrics, concept of Reader was added recently. With reader one can call ReadAndExport to export all data on program termination. It would work same as flush.

Instrument interceptor/opencensus with stats to record the number of received spans. This is accomplished by a helper to process spans that sends over spans even if the number of spans is 0. Also record with tag_key "opencensus_interceptor" whose value is the name of the respective interceptor. The test to ensure that 0 length spans are also added is currently disabled because: * Issue census-instrumentation/opencensus-go#862 is not yet implemented which requests that the OpenCensus-Go stats worker provide a method Flush to flush all data. Without it, metrics will contain stale data from previous tests. Add tests to lock-in this behavior. Also replace grpc.NewServer with a stats enabled server helper call. This enables tracing and metrics for each gRPC server by replacing naked usages of grpc.NewServer with the new helper function: internal.GRPCServerWithObservabilityEnabled Updates census-instrumentation#63

marwan-at-work · 2019-11-02T20:27:39Z

Hi @odeke-em, I noticed one of your commits mentioned in this issue addresses this problem directly, so I'm just wondering if you'll eventually open it as a PR or if there's anything blocking it. Thank you! 🎉

odeke-em · 2019-11-02T20:41:07Z

Hey Marwan, Thank you for the ping! I did open up a PR but it got blocked because there was no consensus/movement to add it via OpenCensus Specs and I believe these libraries are now on auto pilot mode and no longer adding features so unfortunately I don’t think it’ll be done. My PR was sent for over a year.

…

On Sat, Nov 2, 2019 at 1:27 PM Marwan Sulaiman ***@***.***> wrote: Hi @odeke-em <https://github.com/odeke-em>, I noticed one of the commits mentioned in this issue addresses this issue directly, just wondering if you'll eventually open it as a PR or if there's anything blocking it. Thank you! 🎉 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#862?email_source=notifications&email_token=ABFL3V72S2566O257GETAJLQRXPEDA5CNFSM4FOTOTXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC5EJYI#issuecomment-549078241>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFL3V47TL2GLTABDLWOVDDQRXPEDANCNFSM4FOTOTXA> .

marwan-at-work · 2019-11-02T21:40:37Z

@odeke-em Thank you for the response. This seems to me more of a bug than a feature since OpenCensus swallows metrics when a process exists.

In the same spirit, does that mean all other features and bug requests are frozen?

I recently opened this issue which, if my assumption is correct, would be quite noteworthy to address: #1181

Thanks again :)

odeke-em changed the title ~~Make it possible for opencensus to pass all remaining data to registered exporters when program exits.~~ stats: add Flush to immediately report all collected data points regardless of buffering Sep 20, 2018

odeke-em changed the title ~~stats: add Flush to immediately report all collected data points regardless of buffering~~ stats/view: add Flush to immediately report all collected data points regardless of buffering Sep 20, 2018

odeke-em linked a pull request Sep 21, 2018 that will close this issue

stats/view: implement Flush #922

Open

rakyll mentioned this issue Sep 21, 2018

stats Flush not working census-ecosystem/opencensus-go-exporter-stackdriver#43

Open

rakyll self-assigned this Sep 21, 2018

rakyll added a commit to rakyll/opencensus-go that referenced this issue Sep 21, 2018

Add Flush to force aggregate in-memory buffer

728691b

Fixes census-instrumentation#862.

rakyll mentioned this issue Sep 21, 2018

Add Flush to force aggregate in-memory buffer #923

Closed

odeke-em mentioned this issue Oct 5, 2018

interceptor/opencensus: record number of received spans census-instrumentation/opencensus-service#65

Merged

cyriltovena mentioned this issue Dec 22, 2018

How to test/use correctly OpenCensus stats in an application using opencensus-go? #462

Open

songy23 mentioned this issue Jan 24, 2019

Mechanism to explicitly flush stats to exporters #773

Closed

rghetia added the feature-request label May 6, 2019

rghetia added the P2 label May 6, 2019

This was referenced Mar 28, 2022

Wathola Tracing for upgrade tests knative/eventing#6219

Merged

Make sure OpenCensusTracer from tracing package can be correctly shut down (flushed) knative/pkg#2475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stats/view: add Flush to immediately report all collected data points regardless of buffering #862

stats/view: add Flush to immediately report all collected data points regardless of buffering #862

lychung83 commented Aug 9, 2018

odeke-em commented Sep 4, 2018

rakyll commented Sep 4, 2018

odeke-em commented Sep 4, 2018

rakyll commented Sep 4, 2018

odeke-em commented Sep 4, 2018

lychung83 commented Sep 5, 2018

odeke-em commented Sep 20, 2018

razvanh commented Sep 21, 2018 •

edited

semistrict commented Sep 25, 2018

rghetia commented Mar 27, 2019

marwan-at-work commented Nov 2, 2019 •

edited

odeke-em commented Nov 2, 2019 via email

marwan-at-work commented Nov 2, 2019

stats/view: add Flush to immediately report all collected data points regardless of buffering #862

stats/view: add Flush to immediately report all collected data points regardless of buffering #862

Comments

lychung83 commented Aug 9, 2018

odeke-em commented Sep 4, 2018

rakyll commented Sep 4, 2018

odeke-em commented Sep 4, 2018

rakyll commented Sep 4, 2018

odeke-em commented Sep 4, 2018

lychung83 commented Sep 5, 2018

odeke-em commented Sep 20, 2018

razvanh commented Sep 21, 2018 • edited

semistrict commented Sep 25, 2018

rghetia commented Mar 27, 2019

marwan-at-work commented Nov 2, 2019 • edited

odeke-em commented Nov 2, 2019 via email

marwan-at-work commented Nov 2, 2019

razvanh commented Sep 21, 2018 •

edited

marwan-at-work commented Nov 2, 2019 •

edited