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

ability to share default registry #9

Merged
merged 3 commits into from
May 31, 2019
Merged

ability to share default registry #9

merged 3 commits into from
May 31, 2019

Conversation

apesternikov
Copy link
Contributor

Fixes #8

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@apesternikov
Copy link
Contributor Author

I signed it

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@apesternikov
Copy link
Contributor Author

Any chance to have this reviewed?

@bogdandrutu bogdandrutu requested a review from rghetia May 29, 2019 13:30
Copy link
Contributor

@rghetia rghetia left a comment

Choose a reason for hiding this comment

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

Overall looks good. couple of nits.

t.Fatalf("failed to create views: %v", err)
}
defer view.Unregister(v)
view.SetReportingPeriod(time.Millisecond)
Copy link
Contributor

Choose a reason for hiding this comment

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

since prometheus exporter is using new metricexport.Reader interface it simply reads when request is made to /metrics endpoint. So setting reporting period is not necessary. This was required before when Oc library used to push metrics to prometheus exporter.

var output string
for {
time.Sleep(10 * time.Millisecond)
if i == 1000 {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: may be 10 is sufficient to declare it failed. Oc counter should be available right away. Not sure about Prometheus counter though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review. All updates are done, please take a look.

@rghetia rghetia merged commit 71649c8 into census-ecosystem:master May 31, 2019
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.

Ability to use default prometheus Register
3 participants