Skip to content

Adding more documentation around examples#277

Merged
c24t merged 11 commits intoopen-telemetry:masterfrom
codeboten:examples
Nov 16, 2019
Merged

Adding more documentation around examples#277
c24t merged 11 commits intoopen-telemetry:masterfrom
codeboten:examples

Conversation

@codeboten
Copy link
Copy Markdown
Contributor

Adding examples with README files. Basing this off the opentelemetry-js/examples content for consistency.

Alex Boten added 4 commits November 7, 2019 13:35
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
@c24t
Copy link
Copy Markdown
Member

c24t commented Nov 7, 2019

Heads up that isort complains about some of these files. E.g., it would change:

diff --git a/examples/http/server.py b/examples/http/server.py
index fd3d5fd..50f0168 100644
--- a/examples/http/server.py
+++ b/examples/http/server.py
@@ -1,14 +1,16 @@
-import flask
 import os
-import requests

+import flask
+import requests
 from opentelemetry import trace
 from opentelemetry.ext import http_requests
 from opentelemetry.ext.jaeger import JaegerSpanExporter
 from opentelemetry.ext.wsgi import OpenTelemetryMiddleware
 from opentelemetry.sdk.trace import Tracer
-from opentelemetry.sdk.trace.export import ConsoleSpanExporter
-from opentelemetry.sdk.trace.export import BatchExportSpanProcessor
+from opentelemetry.sdk.trace.export import (
+    BatchExportSpanProcessor,
+    ConsoleSpanExporter,
+)

We had to use this isort configuration to use black, but I think this actually makes imports less readable, and think we might actually prefer the single-line-import style for examples. We may want to disable isort for examples altogether.

Copy link
Copy Markdown
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

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

LGTM, I think it's a great idea to have top-level examples with nice README files like this.

We may want to move these off jaeger in the future, or include links to other exporter examples, but this looks great for now.

Signed-off-by: Alex Boten <aboten@lightstep.com>
@codeboten
Copy link
Copy Markdown
Contributor Author

We had to use this isort configuration to use black, but I think this actually makes imports less readable, and think we might actually prefer the single-line-import style for examples. We may want to disable isort for examples altogether.

This ended up giving me problems with black... I just changed the code to multi line import for now

Signed-off-by: Alex Boten <aboten@lightstep.com>
@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 7, 2019

Codecov Report

Merging #277 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #277   +/-   ##
=======================================
  Coverage   85.74%   85.74%           
=======================================
  Files          33       33           
  Lines        1620     1620           
  Branches      183      183           
=======================================
  Hits         1389     1389           
  Misses        184      184           
  Partials       47       47

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 fcdd9fe...b57e2c1. Read the comment docs.

Copy link
Copy Markdown
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

LGTM overall.

My only blocking point is that the http example you introduced overlaps with https://github.com/open-telemetry/opentelemetry-python/tree/master/examples/trace, those should be combined together.


from opentelemetry import trace
from opentelemetry.context import Context
from opentelemetry.ext.jaeger import JaegerSpanExporter
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this import has to be moved under the conditional because somebody trying only the console exporter will not have installed the Jaeger one.
Applies for other examples as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good point, updated the examples

Click on the trace to view its details.

<p align="center"><img src="./images/jaeger-ui-detail.png?raw=true"/></p>
## Useful links
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's not rendering properly, is an empty line before required?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

return "hello"


if __name__ == "__main__":
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am wondering if this conditional is really required.

@mauriciovasquezbernal
Copy link
Copy Markdown
Member

I forgot to say that a link in the main readme could be nice. We have a link but only for the example-app, (just before the contributing section https://github.com/open-telemetry/opentelemetry-python/blob/master/README.md#contributing), maybe this should point to all the examples instead.

- combining http & trace examples into single example
- adding README in examples
- adding link to examples in the main README
- updating example code to conditional import the JaegerSpanExporter

Signed-off-by: Alex Boten <aboten@lightstep.com>
Copy link
Copy Markdown
Contributor Author

@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.

I updated the main README with a link to the examples folder, which now contains a README with some information about the examples https://github.com/open-telemetry/opentelemetry-python/tree/85dc4a63592d5bcdede2d508efcc3de67b8740e3/examples

)

if os.getenv("EXPORTER") == "jaeger":
from opentelemetry.ext.jaeger import JaegerSpanExporter
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any reason not to put this in the same conditional as the exporter creation below?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about doing that as well. The only reason I didn't was to keep the imports separate from the rest of the code, but i don't feel strongly one way or another. I'll update it, it makes the code simpler

Signed-off-by: Alex Boten <aboten@lightstep.com>
Copy link
Copy Markdown
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

I think the code looks great! But I do agree with @mauriciovasquezbernal that we need to keep the examples concise and not duplicate.

There's already examples nearly identical to this in opentelemetry-example-app and in trace. Please reconcile this one with at least one of those so we don't have a third example of the same thing.

Also unit tests would be great, so we could catch the examples breaking.

@codeboten
Copy link
Copy Markdown
Contributor Author

@toumorokoshi I combined the trace example into http to keep the naming consistent with the opentelemetry-js http example

Alex Boten added 3 commits November 12, 2019 23:59
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
@toumorokoshi
Copy link
Copy Markdown
Member

LGTM! thanks for adding tests, and thanks for considering standardization by normalizing our names with the js project!

@c24t c24t merged commit 348889d into open-telemetry:master Nov 16, 2019
@codeboten codeboten deleted the examples branch October 6, 2020 15:34
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants