Skip to content

Fix to run the Auto instrumentation example in the docs #1435

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.

Sign up for GitHub

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

Merged
merged 24 commits into from
Dec 8, 2020
Merged

Fix to run the Auto instrumentation example in the docs #1435

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
c8580b2
Merge pull request #1 from open-telemetry/master
dmarar Nov 27, 2020
207502b
Fix to run the auto-instrumentation example successfully
dmarar Nov 29, 2020
144e85c
reverting the changes in ConsoleSpanExporter, setup.cfg
dmarar Nov 29, 2020
18f3375
Merge pull request #2 from open-telemetry/master
dmarar Nov 30, 2020
5db7f6c
Merge pull request #3 from dmarar/master
dmarar Nov 30, 2020
ffb7ae3
changes to include a consolespanexporter and consolemetricsexporter a…
dmarar Nov 30, 2020
f60c0be
Change as per review comments to replace **kwargs with service_name
dmarar Dec 1, 2020
7af07ba
moving the service_name as the first parameter in the ConsoleSpanExpo…
dmarar Dec 1, 2020
8039aeb
Merge branch 'master' of https://github.com/open-telemetry/openteleme…
dmarar Dec 3, 2020
374bec9
Merge branch 'master' into auto-instru-example-fix
lzchen Dec 3, 2020
88926f5
changes to add the service_name to the exported span
dmarar Dec 3, 2020
acd9ad1
Merge branch 'master' into auto-instru-example-fix
lzchen Dec 3, 2020
698b6fb
Merge branch 'master' into auto-instru-example-fix
lzchen Dec 4, 2020
6063a13
pass the service name to the exported span
dmarar Dec 4, 2020
a85f9f1
Merge pull request #4 from dmarar/new_test
dmarar Dec 4, 2020
6c47b67
update the setup.cfg to the correct path of ConsoleMetricsExporter
dmarar Dec 4, 2020
aa41b03
Merge pull request #5 from dmarar/new_test
dmarar Dec 4, 2020
3b0e945
Merge pull request #6 from open-telemetry/master
dmarar Dec 4, 2020
715c08e
Merge pull request #7 from open-telemetry/master
dmarar Dec 7, 2020
2ed540e
Merge pull request #8 from dmarar/master
dmarar Dec 7, 2020
6619a37
revert the changes to add the service_name to the exported span.
dmarar Dec 7, 2020
62d7ed0
Merge pull request #9 from open-telemetry/master
dmarar Dec 8, 2020
9482378
Merge branch 'master' into auto-instru-example-fix
lzchen Dec 8, 2020
6496413
Merge branch 'master' into auto-instru-example-fix
lzchen Dec 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions docs/examples/auto-instrumentation/README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,8 @@ Manually instrumented server
def server_request():
with tracer.start_as_current_span(
"server_request",
parent=propagators.extract(
lambda dict_, key: dict_.get(key, []), request.headers
)["current-span"],
context=propagators.extract(DictGetter(), request.headers
),
):
print(request.args.get("param"))
return "served"
Expand Down Expand Up @@ -148,7 +147,7 @@ and run the following command instead:

.. code:: sh

$ opentelemetry-instrument python server_uninstrumented.py
$ opentelemetry-instrument -e console_span,console_metrics python server_uninstrumented.py

In the console where you previously executed ``client.py``, run the following
command again:
Expand Down
5 changes: 2 additions & 3 deletions docs/examples/auto-instrumentation/server_instrumented.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
ConsoleSpanExporter,
SimpleExportSpanProcessor,
)
from opentelemetry.trace.propagation.textmap import DictGetter

app = Flask(__name__)

Expand All @@ -36,9 +37,7 @@
def server_request():
with tracer.start_as_current_span(
"server_request",
parent=propagators.extract(
lambda dict_, key: dict_.get(key, []), request.headers
)["current-span"],
context=propagators.extract(DictGetter(), request.headers),
kind=trace.SpanKind.SERVER,
attributes=collect_request_attributes(request.environ),
):
Expand Down
3 changes: 3 additions & 0 deletions opentelemetry-sdk/setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ opentelemetry_tracer_provider =
sdk_tracer_provider = opentelemetry.sdk.trace:TracerProvider
opentelemetry_propagator =
b3 = opentelemetry.sdk.trace.propagation.b3_format:B3Format
opentelemetry_exporter =
console_span = opentelemetry.sdk.trace:ConsoleSpanExporter
console_metrics = opentelemetry.sdk.metrics:ConsoleMetricsExporter
Copy link
Contributor

@adamantike adamantike Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This fails when testing locally, with the following error:

ImportError: module 'opentelemetry.sdk.trace' has no attribute 'ConsoleSpanExporter'

Currently, there's no ConsoleSpanExporter import in opentelemetry/sdk/trace/__init__.py, so the line should be changed to:

console_span = opentelemetry.sdk.trace.export:ConsoleSpanExporter

This is not required for the metrics one, as ConsoleMetricsExporter exists in opentelemetry/sdk/metrics/__init__.py, but could be changed too, to avoid issues in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the issue exists for ConsoleMetricsExporter as well so that would also have to be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes have been made. @lzchen could you please review and let me know your thoughts?


[options.extras_require]
test =
1 change: 1 addition & 0 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,7 @@ def __init__(
out: typing.IO = sys.stdout,
formatter: typing.Callable[[Span], str] = lambda span: span.to_json()
+ os.linesep,
**kwargs
Copy link
Contributor Author

@dmarar dmarar Nov 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This change will be required else the code in init_tracing method

provider.add_span_processor(
            BatchExportSpanProcessor(exporter_class(**exporter_args))
        )

fails with error "unexpected argument" service name.

Copy link
Member

Choose a reason for hiding this comment

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

got it! thanks.

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we only add service_name as an argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could add a default parameter service_name=None. I thought of adding **kwargs because in future if additional parameters get passed, we might have to come back and change again.
I could still go ahead and make the change suggested. Please let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

**kwargs is more robust but this service_name requirement is restriction placed on the constructors by an external component (auto-instrumentation), which is not ideal. If changes happen in the future if we want to pass additional configurable parameters via auto-instrumentation, all of these parameters must be added to the separate exporters that currently are supported (zipkin, jaeger, etc.), so this would have to be addressed regardless. For now, it is better to keep these parameters explicit so we know which ones are available and are needed to exist for this auto-instrumentation feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lzchen ok got it. Will make the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lzchen - The tests seems to fail with an error "GitHub Actions has encountered an internal error when running your job.
". I ran all the tests locally (except pypy3) and they were passing.
Any pointers to understand whether its my failure or a general failure would be helpful.

):
self.out = out
self.formatter = formatter
Expand Down