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

Add proper metric flush #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

quentin-cha
Copy link
Owner

proposed fix for google#1920

Copy link

@Harwayne Harwayne left a comment

Choose a reason for hiding this comment

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

To be sure I understand, the intention is to only submit vendor/contrib.go.opencensus.io/exporter/stackdriver/stats.go and vendor/go.opencensus.io/metric/metricexport/reader.go, correct? The changes to other files seem to be mostly to speed up triggering the bug, rather than something needed for the fix, and therefore aren't something you intend to submit, right?

If my assumption is correct, then those two vendored files LGTM. I especially appreciate the in-depth comment in stats.go.

@quentin-cha
Copy link
Owner Author

To be sure I understand, the intention is to only submit vendor/contrib.go.opencensus.io/exporter/stackdriver/stats.go and vendor/go.opencensus.io/metric/metricexport/reader.go, correct? The changes to other files seem to be mostly to speed up triggering the bug, rather than something needed for the fix, and therefore aren't something you intend to submit, right?

If my assumption is correct, then those two vendored files LGTM. I especially appreciate the in-depth comment in stats.go.

Yes you are correct, only the two .go files in the vendor directories are part of the fix. Thanks for the LGTM

@github-actions
Copy link

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

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

Successfully merging this pull request may close these issues.

2 participants