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

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

Closed
mgencur opened this issue Mar 29, 2022 · 3 comments · Fixed by #2566
Closed

Comments

@mgencur
Copy link
Contributor

mgencur commented Mar 29, 2022

The problem is that currently the components that use SetupStaticPublishing or SetupDynamicPublishing can drop some Spans/Traces when being shut down or evicted (e.g. during upgrades).
The goal is to prevent dropping buffered traces before they're sent as indicated by upstream issue in OpenCensus.

The OpenCensusTracer has a Finish function that can be called to properly Close the OpenZipkin http reporter and flush buffered traces. However, the SetupStaticPublishing/SetupDynamicPublishing functions don't make the OpenCensusTracer available so that anyone can call the Finish function. Another problem is that the Finish function passes nil which triggers nil pointer exception when calling the Finish function.

I've verified that calling the Finish function on the returned OpenCensusTracer sends all remaining traces and makes them available in Zipkin backend. It is only required to pass &config.Config{} on this line instead of nil.

It would be good to enforce calling Finish before components are shutdown as long as there's no sleep time in that component by default before it is shut down or evicted (I think some components have the sleep time such as Knative Serving Activator).

Related discussion here and here

Expected Behavior

It is possible to flush traces on demand (possibly before shutting down a component).

Actual Behavior

Traces that were buffered before the component is shutdown are not properly sent to tracing backend.

Additional Info

@cardil
Copy link
Contributor

cardil commented Mar 29, 2022

This looks like super easy to fix.

I think we should force change the SetupStaticPublishing or SetupDynamicPublishing funcs, so they will return the (*tracing.OpenCensusTracer, error).

Such incompatible change will enforce all Knative components to align, and in the process fix this issue, by calling Finish on shutdown.

@github-actions
Copy link
Contributor

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 28, 2022
@mgencur
Copy link
Contributor Author

mgencur commented Jun 29, 2022

/reopen

@github-actions github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 30, 2022
mgencur added a commit to mgencur/eventing-kafka-broker that referenced this issue Aug 22, 2022
The issue knative/pkg#2475 has been resolved.
It's not necessary to send multiple events anymore. Each event should be
exported to Zipking because of the new fix for tracer shutdown.
mgencur added a commit to mgencur/eventing-kafka-broker that referenced this issue Aug 22, 2022
The issue knative/pkg#2475 has been resolved.
It's not necessary to send multiple events anymore. Each event should be
exported to Zipking because of the new fix for tracer shutdown.
mgencur added a commit to mgencur/eventing-kafka-broker that referenced this issue Aug 22, 2022
The issue knative/pkg#2475 has been resolved.
It's not necessary to send multiple events anymore. Each event should be
exported to Zipking because of the new fix for tracer shutdown.
mgencur added a commit to mgencur/eventing-kafka-broker that referenced this issue Aug 23, 2022
The issue knative/pkg#2475 has been resolved.
It's not necessary to send multiple events anymore. Each event should be
exported to Zipking because of the new fix for tracer shutdown.
mgencur added a commit to mgencur/eventing-kafka-broker that referenced this issue Sep 5, 2022
The issue knative/pkg#2475 has been resolved.
It's not necessary to send multiple events anymore. Each event should be
exported to Zipking because of the new fix for tracer shutdown.
knative-prow bot pushed a commit to knative-extensions/eventing-kafka-broker that referenced this issue Sep 6, 2022
* Do not call deprecated eventshub.AddTracing

* Remove workaround for incorrect tracing shutdown

The issue knative/pkg#2475 has been resolved.
It's not necessary to send multiple events anymore. Each event should be
exported to Zipking because of the new fix for tracer shutdown.

* Update reconciler-test dependency
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

Successfully merging a pull request may close this issue.

2 participants