Improve console span exporter #505

mauriciovasquezbernal · 2020-03-17T21:10:43Z

The current version of the exporter prints everything in a single line, making
it difficult to read. It's also missing events, links and attributes.

This commit changes the console span exporter to use multiple lines and also
adds the missing information about attributes, events and links.

Fixes: #478

I started thinking it was a super easy task, then I realized it was more complex than I anticipated. I ended up with this version, I would like to know opinions on this.

Edit: I realized It was getting too messy by handling all the indentation logic and so on. I created a new version that uses json to format the spans.

Example code:

with tracer.start_as_current_span("foo") as foo:
    time.sleep(0.1)
    foo.set_attribute("my_atribbute", True)
    foo.set_attribute("speed", 100.0)
    foo.add_event("event in foo", {"name": "foo1"})
    foo.add_event("another event ", {"busy": "yes"})
    with tracer.start_as_current_span(
        "bar", links=[trace.Link(foo.get_context())]
    ):
        time.sleep(0.2)

    time.sleep(0.1)

Example output:

{
    "name": "bar",
    "context": {
        "trace_id": "0x16c0898305a3c8502010bda5f294f1af",
        "span_id": "0x7c7ed84c9fe216a6",
        "trace_state": "{}"
    },
    "kind": "SpanKind.INTERNAL",
    "parent_id": "0x4110ce5e5d865531",
    "start_time": "2020-03-19T01:38:43.874587Z",
    "end_time": "2020-03-19T01:38:44.074997Z",
    "attributes": {},
    "events": [],
    "links": [
        {
            "context": {
                "trace_id": "0x16c0898305a3c8502010bda5f294f1af",
                "span_id": "0x4110ce5e5d865531",
                "trace_state": "{}"
            },
            "attributes": {}
        }
    ]
}
{
    "name": "foo",
    "context": {
        "trace_id": "0x16c0898305a3c8502010bda5f294f1af",
        "span_id": "0x4110ce5e5d865531",
        "trace_state": "{}"
    },
    "kind": "SpanKind.INTERNAL",
    "parent_id": null,
    "start_time": "2020-03-19T01:38:43.773931Z",
    "end_time": "2020-03-19T01:38:44.175258Z",
    "attributes": {
        "my_atribbute": true,
        "speed": 100.0
    },
    "events": [
        {
            "name": "event in foo",
            "timestamp": "2020-03-19T01:38:43.874393Z",
            "attributes": {
                "name": "foo1"
            }
        },
        {
            "name": "another event ",
            "timestamp": "2020-03-19T01:38:43.874436Z",
            "attributes": {
                "busy": "yes"
            }
        }
    ],
    "links": []
}

codecov-io · 2020-03-17T21:25:18Z

Codecov Report

Merging #505 into master will decrease coverage by 0.63%.
The diff coverage is 57.40%.

@@            Coverage Diff             @@
##           master     #505      +/-   ##
==========================================
- Coverage   89.33%   88.70%   -0.64%     
==========================================
  Files          43       43              
  Lines        2176     2266      +90     
  Branches      248      258      +10     
==========================================
+ Hits         1944     2010      +66     
- Misses        161      178      +17     
- Partials       71       78       +7

Impacted Files	Coverage Δ
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py	`86.31% <57.40%> (-6.21%)`	⬇️
...lemetry/correlationcontext/propagation/__init__.py	`95.34% <0.00%> (-0.11%)`	⬇️
...i/src/opentelemetry/correlationcontext/__init__.py	`100.00% <0.00%> (ø)`
...elemetry-api/src/opentelemetry/metrics/__init__.py	`93.50% <0.00%> (+0.45%)`	⬆️
...etry-sdk/src/opentelemetry/sdk/metrics/__init__.py	`94.87% <0.00%> (+0.46%)`	⬆️

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 d870abf...82d1d88. Read the comment docs.

The current version of the exporter prints everything in a single line, making it difficult to read. It's also missing events, links and attributes. This commit changes the console span exporter to use multiple lines and also adds the missing information about attributes, events and links.

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

ocelotl

Just a suggestion here 👍

toumorokoshi · 2020-03-19T15:24:00Z

One note I'll just call out is that by providing a json representation via the console exporters, users will begin to depend on the json output of the exporter.

On json in particular, it may be a better practice to create a JsonSpanExporter, and then use that in the examples (of course then people would ask why have an uglier consolespanexporter).

mauriciovasquezbernal · 2020-03-19T18:46:38Z

What's the risk if people start relying on the output from the console exporter?, I think it'll only change if the internals of the span change, otherwise I think it should be quite stable. Anyway, we could make it clear that the output should not be used and that we don't guarantee any stability there.

What I don't want to do is to expend time trying to create our custom representation, I tried and I found that it is very difficult to create a nice and complete one, that's the reason I decided to switch to a standard like json.

ocelotl · 2020-03-19T19:03:04Z

What's the risk if people start relying on the output from the console exporter?, I think it'll only change if the internals of the span change, otherwise I think it should be quite stable. Anyway, we could make it clear that the output should not be used and that we don't guarantee any stability there.

What I don't want to do is to expend time trying to create our custom representation, I tried and I found that it is very difficult to create a nice and complete one, that's the reason I decided to switch to a standard like json.

Maybe the JSON-like output of the standard formatter as it is now can be made less JSON looking by some easy to implement change, in order to make it less likely that our users would expect for this output to be reliable JSON. Something like this:

from json import dumps
from re import sub

data = {1: 2, 3: {4: 5, 6: {3: 6}}}
print(dumps(data, indent=4))
print(sub("\s*{", "", sub("\s*}", "", dumps(data, indent=4))))

{
    "1": 2,
    "3": {
        "4": 5,
        "6": {
            "3": 6
        }
    }
}

    "1": 2,
    "3":
        "4": 5,
        "6":
            "3": 6

toumorokoshi · 2020-04-02T23:32:19Z

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

-            util.ns_to_iso_str(self.start_time) if self.start_time else "None",
-            util.ns_to_iso_str(self.end_time) if self.end_time else "None",
-        )
+        def format_context(context):


do these need to be inline functions? why not utility functions outside of the string?

I haven't done the investigation, but I wonder if that's adding overhead by requiring lambdas to be generated dynamically every time str is called.

I moved them as static functions on the class.

toumorokoshi · 2020-04-02T23:33:09Z

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

-            util.ns_to_iso_str(self.end_time) if self.end_time else "None",
-        )
+        def format_context(context):
+            x_ctx = OrderedDict()


is this necessary? also in python3.7 and above, dicts are always ordered.

I think so. The main purpose of this exporter is for debugging, so I think guaranteeing that the output order is deterministic will help a lot.

- move helpers methods to be part class instead of inlined functions - use to_json() instead of __str__()

mauriciovasquezbernal · 2020-04-13T12:58:15Z

I updated the PR to use to_json() instead of __str__(). I'd like to get input on this as well.

mauriciovasquezbernal · 2020-04-17T11:27:27Z

I closed it by mistake while cleaning up some branches. It's open and waiting for more reviews.

codeboten

Just a question re. the http test, otherwise this looks great.

codeboten · 2020-03-30T16:34:20Z

docs/examples/http/tests/test_http.py

@@ -32,7 +32,7 @@ def test_http(self):
        output = subprocess.check_output(
            (sys.executable, test_script)
        ).decode()
-        self.assertIn('name="/"', output)
+        self.assertIn('"name":', output)


Any reason this is changing the test to not check for /?

Thanks for catching that, updated.

ocelotl · 2020-04-17T17:02:17Z

Approving this, I think that we can sort out formatting details later, but this is a fantastic step forward on the very hard to read console spans we have now.

ocelotl

LGTM

The current version of the exporter prints everything in a single line, making it difficult to read. It's also missing events, links and attributes. This commit changes the console span exporter to use multiple lines and also adds the missing information about attributes, events and links.

@obecny

* chore: add @obecny in CODEOWNERS * chore: update approvers list

mauriciovasquezbernal requested a review from a team March 17, 2020 21:10

mauriciovasquezbernal force-pushed the mauricio/improve-console-span-exporter branch from b0f139a to 06d1713 Compare March 18, 2020 12:55

ocelotl reviewed Mar 18, 2020

View reviewed changes

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

ocelotl reviewed Mar 18, 2020

View reviewed changes

mauriciovasquezbernal added 2 commits March 18, 2020 20:33

format using json

828f0d5

fix tests

82d1d88

mauriciovasquezbernal added the exporters label Mar 19, 2020

toumorokoshi reviewed Apr 9, 2020

View reviewed changes

Update console span exporter

daadaed

- move helpers methods to be part class instead of inlined functions - use to_json() instead of __str__()

Merge branch 'master' into mauricio/improve-console-span-exporter

fcad0d9

mauriciovasquezbernal closed this Apr 14, 2020

mauriciovasquezbernal deleted the mauricio/improve-console-span-exporter branch April 14, 2020 21:50

mauriciovasquezbernal restored the mauricio/improve-console-span-exporter branch April 17, 2020 11:26

mauriciovasquezbernal reopened this Apr 17, 2020

codeboten approved these changes Apr 17, 2020

View reviewed changes

mauriciovasquezbernal added 2 commits April 17, 2020 11:52

test-http-example: look for span name too

00b21dc

Merge branch 'master' into mauricio/improve-console-span-exporter

ebf891d

ocelotl approved these changes Apr 17, 2020

View reviewed changes

mauriciovasquezbernal added 4 commits April 17, 2020 17:05

fix sdk tests

6fa466e

Add span status

1c95153

Merge branch 'master' into mauricio/improve-console-span-exporter

1702876

fix span status in to_json

7a83567

mauriciovasquezbernal added the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Apr 23, 2020

toumorokoshi merged commit 232bfdd into open-telemetry:master Apr 23, 2020

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020

Propose to add @obecny as the CODEOWNERS. (open-telemetry#505)

f5801d2

* chore: add @obecny in CODEOWNERS * chore: update approvers list

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve console span exporter #505

Improve console span exporter #505

mauriciovasquezbernal commented Mar 17, 2020 •

edited

Loading

codecov-io commented Mar 17, 2020 •

edited

Loading

ocelotl left a comment

toumorokoshi commented Mar 19, 2020

mauriciovasquezbernal commented Mar 19, 2020

ocelotl commented Mar 19, 2020

toumorokoshi Apr 2, 2020

mauriciovasquezbernal Apr 13, 2020

toumorokoshi Apr 2, 2020

mauriciovasquezbernal Apr 13, 2020

mauriciovasquezbernal commented Apr 13, 2020

mauriciovasquezbernal commented Apr 17, 2020

codeboten left a comment

codeboten Mar 30, 2020

mauriciovasquezbernal Apr 17, 2020

ocelotl commented Apr 17, 2020

ocelotl left a comment

Improve console span exporter #505

Improve console span exporter #505

Conversation

mauriciovasquezbernal commented Mar 17, 2020 • edited Loading

codecov-io commented Mar 17, 2020 • edited Loading

Codecov Report

ocelotl left a comment

Choose a reason for hiding this comment

toumorokoshi commented Mar 19, 2020

mauriciovasquezbernal commented Mar 19, 2020

ocelotl commented Mar 19, 2020

toumorokoshi Apr 2, 2020

Choose a reason for hiding this comment

mauriciovasquezbernal Apr 13, 2020

Choose a reason for hiding this comment

toumorokoshi Apr 2, 2020

Choose a reason for hiding this comment

mauriciovasquezbernal Apr 13, 2020

Choose a reason for hiding this comment

mauriciovasquezbernal commented Apr 13, 2020

mauriciovasquezbernal commented Apr 17, 2020

codeboten left a comment

Choose a reason for hiding this comment

codeboten Mar 30, 2020

Choose a reason for hiding this comment

mauriciovasquezbernal Apr 17, 2020

Choose a reason for hiding this comment

ocelotl commented Apr 17, 2020

ocelotl left a comment

Choose a reason for hiding this comment

mauriciovasquezbernal commented Mar 17, 2020 •

edited

Loading

codecov-io commented Mar 17, 2020 •

edited

Loading