Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

doc: add SetReportingPeriod recommendations #910

Merged

Conversation

jeanbza
Copy link
Contributor

@jeanbza jeanbza commented Sep 11, 2018

No description provided.

Copy link
Member

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @jadekler! There is a minor nit, PTAL.

@@ -113,11 +113,15 @@ func record(tags *tag.Map, ms interface{}) {
ms: ms.([]stats.Measurement),
}
defaultWorker.c <- req
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this, this ain't Java 😂

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

// the program. If duration is less than or equal to zero, it enables the
// default behavior.
//
// Note: each exporter makes different promises about what the lowest supported
Copy link
Member

Choose a reason for hiding this comment

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

Nice, thank you! It would be nice to also mail a change to the actual stackdriver exporter repo providing an advisory for this time duration.

Copy link
Member

Choose a reason for hiding this comment

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

@songy23 I see you initially mailed census-ecosystem/opencensus-go-exporter-stackdriver#37 but closed it. Will you perhaps be making an update in light of this new PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the Stackdriver example is already updated in census-ecosystem/opencensus-go-exporter-stackdriver#38.

@jeanbza
Copy link
Contributor Author

jeanbza commented Sep 11, 2018

PTAL

Copy link
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

LGTM

@songy23 songy23 merged commit 53d4387 into census-instrumentation:master Sep 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants