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

gcp/observability: fix End() to cleanup global state correctly #5623

Merged
merged 5 commits into from Sep 7, 2022

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented Aug 31, 2022

Previously, our metrics and tracing calls would not have the buffer flushed in observability.End(), and also keep the exporters registered globally in another package. Thus, tracing and metrics data would possibly not get uploaded to a backend properly. Also, our tests would break when run more than once due to the trailing exporter.

Fixes #5423, #5558, #5559

RELEASE NOTES:

  • gcp/observability: fix End() to cleanup global state correctly

@zasweq zasweq requested a review from dfawley August 31, 2022 23:31
@zasweq zasweq added this to the 1.50 Release milestone Aug 31, 2022
@dfawley dfawley changed the title O11y: Fix Observability cleanup gcp/observability: Fix Observability cleanup Sep 1, 2022
@@ -80,4 +85,18 @@ func Start(ctx context.Context) error {
// Note: this method should only be invoked once.
func End() {
defaultLogger.Close()
if exporter != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create a stopOpenCensus function that does the cleanup needed due to startOpenCensus, and put it right next to it so we can be sure everything pairs up between start/end.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks.

gcp/observability/opencensus.go Show resolved Hide resolved
@dfawley dfawley assigned zasweq and unassigned dfawley Sep 1, 2022
@dfawley dfawley changed the title gcp/observability: Fix Observability cleanup gcp/observability: fix cleanup to flush metrics and tracing buffers and unregister global interceptors Sep 1, 2022
@zasweq zasweq assigned dfawley and unassigned zasweq Sep 1, 2022
gcp/observability/opencensus.go Show resolved Hide resolved
gcp/observability/opencensus.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned zasweq and unassigned dfawley Sep 6, 2022
Copy link
Contributor Author

@zasweq zasweq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review :D!

gcp/observability/opencensus.go Show resolved Hide resolved
gcp/observability/opencensus.go Outdated Show resolved Hide resolved
@zasweq zasweq assigned dfawley and unassigned zasweq Sep 6, 2022
gcp/observability/opencensus.go Show resolved Hide resolved
trace.UnregisterExporter(exporter)
view.UnregisterExporter(exporter)

if sdExporter, ok := exporter.(*stackdriver.Exporter); ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not define Flush and Close methods on tracingMetricsExporter instead? Does it make the tests unnecessarily complex?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@dfawley dfawley assigned zasweq and unassigned dfawley Sep 6, 2022
@zasweq zasweq changed the title gcp/observability: fix cleanup to flush metrics and tracing buffers and unregister global interceptors gcp/observability: fix End() to cleanup global state correctly Sep 6, 2022
@zasweq zasweq merged commit 1530d3b into grpc:master Sep 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

observability: data race
2 participants