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

dmarar · 2020-11-29T15:27:10Z

Description

The auto-instrumentation example given in the docs does not work as expected.
The changes were made to make the example work as expected.

Fixes # (issue)
Auto-instrumentation example fix.
Fixes open-telemetry/opentelemetry-specification#1432

Type of change

Code
Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test A

Does This PR Require a Contrib Repo Change?

No

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/
The method interfaces of opentelemetry-instrumentation/ have changed
The method interfaces of test/util have changed
Scripts in scripts/ that were copied over to the Contrib repo have changed
Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in
- pyproject.toml
- isort.cfg
- .flake8
When a new .github/CODEOWNER is added
Major changes to project information, such as in:
- README.md
- CONTRIBUTING.md
Yes. - Link to PR:
No.

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated

merging from opentelemetry/master

linux-foundation-easycla · 2020-11-29T15:27:14Z

The committers are authorized under a signed CLA.

✅ Dilip M (c8580b2, 207502b)

dmarar · 2020-11-29T15:36:47Z

This PR is a solution to fix issue open-telemetry/opentelemetry-specification#1432

Modifying the README.rst accordingly.

toumorokoshi

Thanks! the DictGetter change looks good, see my notes on the -e none.

docs/examples/auto-instrumentation/README.rst

merging from upstream/master

merging from origin/master

…s per review comments.

dmarar · 2020-11-30T19:14:43Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py

@@ -373,6 +373,7 @@ def __init__(
        out: typing.IO = sys.stdout,
        formatter: typing.Callable[[Span], str] = lambda span: span.to_json()
        + os.linesep,
+        **kwargs


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.

got it! thanks.

Shouldn't we only add service_name as an argument?

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.

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

@lzchen ok got it. Will make the changes.

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

toumorokoshi

Great! Thanks for the change.

…rter since some py35-exporter-zipkin test were failing.

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py

dmarar · 2020-12-03T05:27:06Z

@lzchen , Thanks for re-triggering the tests 👍 . All the tests have passed. Please let me know if there is anything I need to do before merging the pull request?

…try-python

adamantike · 2020-12-04T14:41:13Z

opentelemetry-sdk/setup.cfg

+    console_span = opentelemetry.sdk.trace:ConsoleSpanExporter
+    console_metrics = opentelemetry.sdk.metrics:ConsoleMetricsExporter


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.

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

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

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py

fix the unit tests for the above change.

New test

update the setup.cfg to the correct path of ConsoleMetricsExporter

merging from master

adamantike · 2020-12-04T19:18:16Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py

+            span_json = json.loads(self.formatter(span))
+            span_json.update({"service_name": self.service_name})
+            self.out.write(json.dumps(span_json, indent=4))


Is there any guarantee that the user will always provide a formatter that converts the span to JSON? Otherwise, a formatter like lambda span: repr(span) will break this logic.

I don't see an easy way to inject the service_name in the response of span.to_json(), unless the Span class is refactored, or Span.attributes is used (but that would mutate the Span object).

we cannot inject anything to the span after its closed. If my understanding is correct consolespanexporter is only for writing output to the console (for debugging) and its not going to be used in production. i think this could be the reason the the formatter is assigned that way so that it just put the output to the console.
Having said this , my understanding is pretty limited wrt to the project as am pretty to new to it.

@adamantike
That is a good point. The only reason why we needed service_name is because the auto-instrumentation requires it to exist in the constructor of configured exporters here. We might want to revisit this restriction as to not have to enforce constructors to be a certain way. The usage of service_name is not mandatory, so for now @dmarar you can just remove the assignment, but just keep it in the constructor.

sure @lzchen will revert the changes.

Thanks for the clarification, @lzchen!

@lzchen, I have made the necessary changes. Could you please take a look?

merging from upstream master

merging from origin master

lzchen · 2020-12-08T14:19:15Z

docs/examples/auto-instrumentation/README.rst

+        "telemetry.sdk.language": "python",
+        "telemetry.sdk.name": "opentelemetry",
+        "telemetry.sdk.version": "0.16b1",
+        "service.name": ""


Suggested change

"service.name": ""

@lzchen , this change is not done by me. it comes from the init_tracing code in components.py :

def init_tracing( exporters: Sequence[SpanExporter], ids_generator: trace.IdsGenerator ): service_name = get_service_name() provider = TracerProvider( resource=Resource.create({"service.name": service_name}), ids_generator=ids_generator(), ) trace.set_tracer_provider(provider)

The outputs should be the same for auto-instrumented and manually instrumented. Since you removed service_name from appearing in the console export, it shouldn't appear for either example.

@lzchen , there was mistake in that output snapshot in previous comment, the service.name was coming irrespective of my change.

The change I made adds an additional "service_name" to the output (please see the output) ( which i removed as per your suggestion)

The code in the init_tracing hasnt been touched by me for any of the changes I did.

My apologies that i realised it very late that output in the previous comment was incorrect.

Please note that "service.name" is not added in the server output if server_instrument.py is run.

merge from upstream master

dmarar · 2020-12-08T16:50:16Z

Thank you @lzchen, @toumorokoshi . Appreciate your time in reviewing and providing inputs.

dmarar and others added 2 commits November 27, 2020 20:45

Merge pull request #1 from open-telemetry/master

c8580b2

merging from opentelemetry/master

Fix to run the auto-instrumentation example successfully

207502b

dmarar requested a review from a team as a code owner November 29, 2020 15:27

dmarar requested review from toumorokoshi and hectorhdzg and removed request for a team November 29, 2020 15:27

reverting the changes in ConsoleSpanExporter, setup.cfg

144e85c

Modifying the README.rst accordingly.

toumorokoshi suggested changes Nov 30, 2020

View reviewed changes

docs/examples/auto-instrumentation/README.rst Outdated Show resolved Hide resolved

dmarar and others added 3 commits December 1, 2020 00:00

Merge pull request #2 from open-telemetry/master

18f3375

merging from upstream/master

Merge pull request #3 from dmarar/master

5db7f6c

merging from origin/master

changes to include a consolespanexporter and consolemetricsexporter a…

ffb7ae3

…s per review comments.

dmarar commented Nov 30, 2020

View reviewed changes

dmarar requested a review from toumorokoshi November 30, 2020 19:15

toumorokoshi approved these changes Dec 1, 2020

View reviewed changes

dmarar changed the title ~~[WIP] Fix to run the Auto instrumentation example in the docs~~ Fix to run the Auto instrumentation example in the docs Dec 1, 2020

dmarar added 2 commits December 1, 2020 23:17

Change as per review comments to replace **kwargs with service_name

f60c0be

moving the service_name as the first parameter in the ConsoleSpanExpo…

7af07ba

…rter since some py35-exporter-zipkin test were failing.

lzchen approved these changes Dec 2, 2020

View reviewed changes

lzchen reviewed Dec 2, 2020

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py Show resolved Hide resolved

dmarar and others added 4 commits December 3, 2020 20:40

Merge branch 'master' of https://github.com/open-telemetry/openteleme…

8039aeb

…try-python

Merge branch 'master' into auto-instru-example-fix

374bec9

changes to add the service_name to the exported span

88926f5

Merge branch 'master' into auto-instru-example-fix

acd9ad1

adamantike reviewed Dec 4, 2020

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py Outdated Show resolved Hide resolved

lzchen and others added 2 commits December 4, 2020 10:21

Merge branch 'master' into auto-instru-example-fix

698b6fb

pass the service name to the exported span

6063a13

fix the unit tests for the above change.

dmarar and others added 4 commits December 5, 2020 00:02

Merge pull request #4 from dmarar/new_test

a85f9f1

New test

update the setup.cfg to the correct path of ConsoleMetricsExporter

6c47b67

Merge pull request #5 from dmarar/new_test

aa41b03

update the setup.cfg to the correct path of ConsoleMetricsExporter

Merge pull request #6 from open-telemetry/master

3b0e945

merging from master

adamantike reviewed Dec 4, 2020

View reviewed changes

dmarar requested a review from lzchen December 4, 2020 19:18

dmarar and others added 3 commits December 7, 2020 23:15

Merge pull request #7 from open-telemetry/master

715c08e

merging from upstream master

Merge pull request #8 from dmarar/master

2ed540e

merging from origin master

revert the changes to add the service_name to the exported span.

6619a37

lzchen reviewed Dec 8, 2020

View reviewed changes

lzchen approved these changes Dec 8, 2020

View reviewed changes

dmarar and others added 3 commits December 8, 2020 20:36

Merge pull request #9 from open-telemetry/master

62d7ed0

merge from upstream master

Merge branch 'master' into auto-instru-example-fix

9482378

Merge branch 'master' into auto-instru-example-fix

6496413

lzchen added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Dec 8, 2020

lzchen merged commit 5450e6d into open-telemetry:master Dec 8, 2020

dmarar deleted the auto-instru-example-fix branch December 8, 2020 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dmarar commented Nov 29, 2020 •

edited

Loading

linux-foundation-easycla bot commented Nov 29, 2020 •

edited

Loading

dmarar commented Nov 29, 2020

toumorokoshi left a comment

dmarar Nov 30, 2020 •

edited

Loading

toumorokoshi Dec 1, 2020

dmarar Dec 1, 2020

lzchen Dec 1, 2020

dmarar Dec 1, 2020

lzchen Dec 1, 2020

dmarar Dec 1, 2020

dmarar Dec 2, 2020

toumorokoshi left a comment

dmarar commented Dec 3, 2020

adamantike Dec 4, 2020 •

edited

Loading

lzchen Dec 4, 2020

dmarar Dec 4, 2020

adamantike Dec 4, 2020

dmarar Dec 4, 2020

lzchen Dec 7, 2020 •

edited

Loading

dmarar Dec 7, 2020

adamantike Dec 7, 2020

dmarar Dec 8, 2020

lzchen Dec 8, 2020

dmarar Dec 8, 2020

lzchen Dec 8, 2020

dmarar Dec 8, 2020 •

edited

Loading

dmarar commented Dec 8, 2020

		console_span = opentelemetry.sdk.trace:ConsoleSpanExporter
		console_metrics = opentelemetry.sdk.metrics:ConsoleMetricsExporter

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

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

Conversation

dmarar commented Nov 29, 2020 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Contrib Repo Change?

Checklist:

linux-foundation-easycla bot commented Nov 29, 2020 • edited Loading

dmarar commented Nov 29, 2020

toumorokoshi left a comment

Choose a reason for hiding this comment

dmarar Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

dmarar commented Dec 3, 2020

adamantike Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzchen Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarar Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

dmarar commented Dec 8, 2020

dmarar commented Nov 29, 2020 •

edited

Loading

linux-foundation-easycla bot commented Nov 29, 2020 •

edited

Loading

dmarar Nov 30, 2020 •

edited

Loading

adamantike Dec 4, 2020 •

edited

Loading

lzchen Dec 7, 2020 •

edited

Loading

dmarar Dec 8, 2020 •

edited

Loading