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

Reorganize examples and improve ext packages documentation #483

Merged
merged 13 commits into from
Mar 24, 2020

Conversation

mauriciovasquezbernal
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal commented Mar 13, 2020

Update examples according to #482.

A preview of the resulting documentation is available at https://opentelemetry-python-kinvolk.readthedocs.io/en/reorganize-examples/.

This PR also moves the text on the readmes of the external packages to their
source code as a docstring. This improves the generated documentation and helps
to consolidate the documentation in a single place.

This commit uniforms all the readmes present on each packager, they only contain
a 1 line description, installation instructions and a link to the online
documentation of such package. This small readme will be the one shown on Github
and on PyPI.

The full documentation will be generated by autodoc and stored in readthedocs
or whatever site we choose.

This commit moves the text on the readmes of the external packages to their
source code as a docstring. This improves the generated documentation and helps
to consolidate the documentation in a single place.

This commit uniforms all the readmes present on each packager, they only contain
a 1 line description, installation instructions and a link to the online
documentation of such package. This small readme will be the one shown on Github
and on PyPI.

The full documentation will be generated by autodoc and stored in readthedocs
or whatever site we choose.
This commit changes the examples structure to have the following examples:
- basic meter & tracer
- http integration
- jaeger, prometheus, otcollector{tracer, metrics}
- opentracing shim
@mauriciovasquezbernal mauriciovasquezbernal added the doc Documentation-related label Mar 13, 2020
@mauriciovasquezbernal mauriciovasquezbernal requested a review from a team March 13, 2020 16:02
@codecov-io
Copy link

codecov-io commented Mar 13, 2020

Codecov Report

Merging #483 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #483   +/-   ##
=======================================
  Coverage   89.47%   89.47%           
=======================================
  Files          43       43           
  Lines        2213     2214    +1     
  Branches      250      250           
=======================================
+ Hits         1980     1981    +1     
  Misses        161      161           
  Partials       72       72           
Impacted Files Coverage Δ
...ts/src/opentelemetry/ext/http_requests/__init__.py 89.47% <ø> (ø)
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py 68.14% <ø> (ø)
...-ext-flask/src/opentelemetry/ext/flask/__init__.py 90.00% <100.00%> (+0.20%) ⬆️
...xt-jaeger/src/opentelemetry/ext/jaeger/__init__.py 86.54% <100.00%> (ø)
...xt-zipkin/src/opentelemetry/ext/zipkin/__init__.py 84.41% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbfafa4...b1cd37a. Read the comment docs.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

This is looking great. Just some suggestions and a few nits. At some point we'll need to review the content in all these docs but I suggest we don't do it with this PR. I've tested each example.

Couple of questions:
Reviewing the docs here, it looks like the prometheus doc is empty: https://opentelemetry-python-kinvolk.readthedocs.io/en/reorganize-examples/ext/prometheus/prometheus.html.
It also looks like the example app doesn't have much content https://opentelemetry-python-kinvolk.readthedocs.io/en/reorganize-examples/examples/opentelemetry-example-app/README.html, is that expected?

The opentelemetry-ext-psycopg2 package allows tracing PostgreSQL queries made by the
Psycopg2 library.
The integration with PostgreSQL supports the `Psycopg`_ library and is specified
to ``trace_integration`` using ``'PostgreSQL'``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what this means.

docs/examples/prometheus/README.rst Outdated Show resolved Hide resolved
docs/examples/prometheus/prometheus.py Outdated Show resolved Hide resolved
docs/examples/basic_meter/observer.py Outdated Show resolved Hide resolved
docs/examples/basic_meter/README.rst Outdated Show resolved Hide resolved
python collector.py


The metrics are available in the Prometheus dashboard at http://localhost:9090/graph.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be helpful here to give an example of a metric to look for ie. requests

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@mauriciovasquezbernal
Copy link
Member Author

This PR includes also the changes in #475, given that I received feedback related to those changes here, I'm going to close that one and keep everything in this single PR.

@mauriciovasquezbernal mauriciovasquezbernal changed the title Reorganize examples Reorganize examples and improve ext packages documentation Mar 16, 2020
@mauriciovasquezbernal
Copy link
Member Author

Reviewing the docs here, it looks like the prometheus doc is empty: https://opentelemetry-python-kinvolk.readthedocs.io/en/reorganize-examples/ext/prometheus/prometheus.html.

prometheus-client dependency was missing from docs requirements. Fixed!

It also looks like the example app doesn't have much content https://opentelemetry-python-kinvolk.readthedocs.io/en/reorganize-examples/examples/opentelemetry-example-app/README.html, is that expected?

I have no idea what to do with that example. I don't know what is the added value of that. I'd like to hear others' opinions on this.

@andrewhsu
Copy link
Member

Looks like PR #494 does a more up-to-date Getting Started guide.

Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Great! Just a few minor comments

Co-Authored-By: Diego Hurtado <ocelotl@users.noreply.github.com>
Co-Authored-By: Diego Hurtado <ocelotl@users.noreply.github.com>
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@toumorokoshi
Copy link
Member

@mauriciovasquezbernal thanks for the work! I have a few thoughts on the docs themselves but we (I) can pick those up in future PRs.

The only real thought I have is putting the examples above the integrations. Examples are probably more pertinent to most people as they're learning the codebase.

@toumorokoshi
Copy link
Member

@mauriciovasquezbernal can you merge in changes one last time? then I can merge in.

@codeboten codeboten added the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Mar 24, 2020
@toumorokoshi toumorokoshi merged commit 04a9533 into open-telemetry:master Mar 24, 2020
@toumorokoshi
Copy link
Member

@mauriciovasquezbernal merged! thanks.

@mauriciovasquezbernal mauriciovasquezbernal deleted the reorganize-examples branch April 14, 2020 21:50
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
* fix(http-plugin): move node-sdk to dev deps

* feat: update dependencies

* feat: server implementation and tests

* fix: remove console logger

* fix: add types

* feat(prometheus-exporter): counter and gauge

* feat(prometheus-exporter): implement counter and gauge

* feat(prometheus-exporter): export configuration interface

* fix: linting

* fix: typo

Co-Authored-By: Mayur Kale <mayurkale@google.com>

* chore: document ExporterConfig

* fix: dependencies

* fix: typo

Co-Authored-By: Mayur Kale <mayurkale@google.com>

* chore: document sanitize method and make behavior more strict

* chore: do not use global prom-client registry

* chore: add defaults to description and metric endpoint

* test: export couter, gauge, multiple, and none

* test: sanitize prom metric names

* fix: typo

Co-Authored-By: Mayur Kale <mayurkale@google.com>

* chore: make _server read only

* test: add tests for prom export configuration

* style: make comments descriptive

* test: update tests for label sets

* fix: lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation-related PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants