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

quickstart/python: use Prometheus exporter for metrics tutorials #388

Merged
merged 6 commits into from
Feb 6, 2019

Conversation

PikBot
Copy link
Contributor

@PikBot PikBot commented Oct 12, 2018

quickstart/python: use Prometheus exporter for metrics tutorials

Replace Stackdriver Metrics exporter with Prometheus exporter in metrics quickstart
since Prometheus is easy to setup, free, open sourced.

Updates #343
fixes #353
fixes #354

quickstart/python: use Prometheus exporter for metrics tutorials

Replace Stackdriver Metrics exporter with Prometheus exporter in metrics quickstart
since Prometheus is easy to setup, free, open sourced.

Updates census-instrumentation#343
fixes census-instrumentation#353
@PikBot
Copy link
Contributor Author

PikBot commented Oct 12, 2018

@odeke-em please have a look !

@songy23 songy23 requested a review from odeke-em October 12, 2018 20:47
@odeke-em
Copy link
Member

Thank you for this PR @PikBot!

As I had mentioned, the screenshot of this PR needs to be taken from the Prometheus UI itself and not from the scrape endpoint of the example code. Also the content of your current screenshot shows all zeroes for the value so perhaps please check your code. Please take a look at the OpenCensus-Go metrics quickstart as the rubric for which screenshots to take http://localhost:1313/quickstart/go/metrics/#viewing-your-metrics

@PikBot
Copy link
Contributor Author

PikBot commented Oct 15, 2018

I tried running the example, even that does not seem to produce any results for me. I have created a issue here.

@PikBot
Copy link
Contributor Author

PikBot commented Oct 15, 2018

Thanks @odeke-em ! I tried running the example, even that does not seem to produce any results for me. I have created a issue here.

@odeke-em
Copy link
Member

@PikBot please refresh with the latest OpenCensus Python library changes that fixed the problems that were discovered when writing this guide. @songy23 gave me the confirmation last week.

@odeke-em odeke-em removed the blocked label Jan 21, 2019
@PikBot
Copy link
Contributor Author

PikBot commented Jan 24, 2019

@odeke-em thank you, will refresh ASAP.

@PikBot
Copy link
Contributor Author

PikBot commented Jan 24, 2019

@odeke-em kindly review, I have updated the screenshots and any erroneous code.

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.

Awesome, thank you for working and updating this @PikBot!
We'll need a few more updates to use Count Aggregations instead of m_lines_in measure as per the Go stats quickstart https://opencensus.io/quickstart/go/metrics/#create-views
and also remove m_errors* measure and instead use tags -- or you can entirely remove error measurement for now.

content/quickstart/python/metrics.md Outdated Show resolved Hide resolved
content/quickstart/python/metrics.md Outdated Show resolved Hide resolved
content/quickstart/python/metrics.md Show resolved Hide resolved
content/quickstart/python/metrics.md Outdated Show resolved Hide resolved
content/quickstart/python/metrics.md Outdated Show resolved Hide resolved
content/quickstart/python/metrics.md Outdated Show resolved Hide resolved
content/quickstart/python/metrics.md Outdated Show resolved Hide resolved
content/quickstart/python/metrics.md Outdated Show resolved Hide resolved
@PikBot
Copy link
Contributor Author

PikBot commented Jan 25, 2019

@odeke-em thank you for the review! I have made the changes suggested by you.

@odeke-em
Copy link
Member

odeke-em commented Feb 1, 2019

Hello @PikBot, the final issues with the Prometheus exporter not sanitizing it's metric names was just recently resolved by @songy23 with PR census-instrumentation/opencensus-python#468. @songy23 is there a new Python release on the horizon?

Once that Python release is made, let's update this PR and then ensure that the view names are proper i.e. "foo/bar/rest"

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.

Great, LGTM and thank you @PikBot! Minor nits and then waiting for the OpenCensus Python update to be published and then it is prime-time.

content/quickstart/python/metrics.md Outdated Show resolved Hide resolved
content/quickstart/python/metrics.md Outdated Show resolved Hide resolved
content/quickstart/python/metrics.md Outdated Show resolved Hide resolved
@songy23
Copy link
Contributor

songy23 commented Feb 1, 2019

/cc @c24t for OC-Python release schedule.

For now in the example please use '_' instead of '/' for all view names and tag keys.

@odeke-em
Copy link
Member

odeke-em commented Feb 3, 2019

For now in the example please use '_' instead of '/' for all view names and tag keys.

@songy23, sure. @PikBot please take heed of @songy23's advice but let's open up a follow-up issue to have the view names and tag keys updated. This initial PR is the bootstrap to complete the stats tutorial and then that follow-up issue will be to update them whenever the new release is made.

@PikBot
Copy link
Contributor Author

PikBot commented Feb 4, 2019

@odeke-em , for some reason when I tried running the same code again (after updating the libraries), it produced an error :( . I have addressed the issue here for now census-instrumentation/opencensus-python#480

@PikBot
Copy link
Contributor Author

PikBot commented Feb 6, 2019

@odeke-em please review, have made all the final changes. :) Thanks to @songy23 !

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

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 @PikBot!

However, it is quite odd that the latency screenshot doesn't seem to show the "status"="OK" tag in

@odeke-em
Copy link
Member

odeke-em commented Feb 6, 2019

@PikBot just one more round of running this code, please copy it from this tutorial and re-run, update the screenshots and then we'll be good to go -- this is because there were some views that were missing keys. Thank you.

@songy23
Copy link
Contributor

songy23 commented Feb 6, 2019

Just an FYI I found a few more bugs when working with the latest Prometheus Python client (we're testing against an outdated version in the unit tests). I'll send another PR to OC-Python. That shouldn't block this PR though.

@PikBot
Copy link
Contributor Author

PikBot commented Feb 6, 2019

@odeke-em updated!

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.

Awesome, LGTM and thank you @PikBot!

@odeke-em odeke-em merged commit 24f50b9 into census-instrumentation:master Feb 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

quickstart/python: complete stats tutorial quickstart/python: use open source exporters for tutorials
5 participants