Change status codes from grpc status codes, remove setting status in instrumentations except on ERROR #1282

lzchen · 2020-10-26T19:14:00Z

Changes status canonical codes to comply with spec changes. Also changes behavior for setting status within instrumentations. The general idea is: instrumentations should not set status ever unless if an error happens. Span api/sdk should not set status either, so the only time that the span status is set to OK is by the user.

Renames StatusCanonicalCode to StatusCode: Remove "canonical" adjective from Status Code opentelemetry-specification#1081
Changes default StatusCode to UNSET.
Change UNKNOWN to ERROR
Change http_to_canonical_code to follow https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#status
On span creation, default status has status code StatusCode.UNSET. Do not set status to StatusCode.OK upon span end if not set.
Instrumentations do not set status anymore except on ERROR

TODO:

Fix otlpexporter/opencensusexporter mapping from statuscode once protobuf is updated.
Add record_exception option to span __exit__
Add set_status_on_exception parameter to start_as_current_span

owais

Looks good on first pass. I see comments about removing status setting code from instrumentations. Spec says instrumentation should set status in case of errors. Is the idea that instrumenation should not catch exceptions and let tracer/span context handle the exeptions and record status? That sounds good but it should be a general guideline and not a hard rule as in some cases, it might make sense for instrumentation to set status anyway.

owais · 2020-10-28T11:12:52Z

exporter/opentelemetry-exporter-otlp/src/opentelemetry/exporter/otlp/trace_exporter/__init__.py

@@ -159,8 +160,12 @@ def _translate_links(self, sdk_span: SDKSpan) -> None:

    def _translate_status(self, sdk_span: SDKSpan) -> None:
        if sdk_span.status is not None:
+            # TODO: Update this when the proto definitions are updated to include UNSET and ERROR


Is this TODO required for GA? If so, may be create an issue and link it here?

owais · 2020-10-28T11:15:28Z

...telemetry-instrumentation-aiopg/src/opentelemetry/instrumentation/aiopg/aiopg_integration.py

@@ -109,12 +109,13 @@ async def traced_execution(
            try:
                result = await query_method(*args, **kwargs)
                if span.is_recording():
-                    span.set_status(Status(StatusCanonicalCode.OK))
+                    # TODO: Remove setting status in instrumentation


Suggested change

# TODO: Remove setting status in instrumentation

# TODO: Remove setting OK status in instrumentation

That's not exactly accurate. The TODO is removing any set_status in instrumentations that pertain to OK or UNSET. I should probably name it to remove setting non-ERROR status in instrumentation.

Update: I made the changes and will be just removing the TODOs.

...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py

...on/opentelemetry-instrumentation-celery/src/opentelemetry/instrumentation/celery/__init__.py

toumorokoshi

Thanks! I think the one blocker for me is the field "canonical_code" still being in the codebase. Or let me know if that's intended.

...tation/opentelemetry-instrumentation-aiohttp-client/tests/test_aiohttp_client_integration.py

...telemetry-instrumentation-aiopg/src/opentelemetry/instrumentation/aiopg/aiopg_integration.py

...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py

...ry-instrumentation-elasticsearch/src/opentelemetry/instrumentation/elasticsearch/__init__.py

...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py

opentelemetry-api/src/opentelemetry/trace/status.py

opentelemetry-api/CHANGELOG.md

lzchen · 2020-10-28T15:18:55Z

@owais

Is the idea that instrumenation should not catch exceptions and let tracer/span context handle the exeptions and record status?

No that comment about instrumentations not recording status is few lines of code cleanup left to do for the instrumentations that I left so to not bloat up the PR (like here: https://github.com/open-telemetry/opentelemetry-python/pull/1282/files#r513361923). The general idea is: instrumentations should not set status ever unless if an error happens. Span api/sdk should not set status either, so the only time that the span status is set to OK is by the user. I updated the description to address this.

lzchen · 2020-10-28T15:36:46Z

@owais

No that comment about instrumentations not recording status is few lines of code cleanup left to do for the instrumentations that I left so to not bloat up the PR

Update: I made the changes and will be just removing the TODOs.

…try-python into status

toumorokoshi

Thanks! I'd not the one change to eachdist that may be worth reverting.

toumorokoshi · 2020-10-28T20:16:54Z

tox.ini

@@ -369,7 +369,7 @@ commands_pre =
  python scripts/eachdist.py install --editable --with-test-deps

 commands =
-  python scripts/eachdist.py lint --check-only
+  python scripts/eachdist.py lint


will this command still fail on fixing styling commands? I think the reason this was --check-only was to use that tox command for CI, rather than for users trying to run lint / formatting rules.

Sorry left it in by accident =p

toumorokoshi · 2020-10-28T20:18:00Z

opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py

    """Converts an HTTP status code to an OpenTelemetry canonical status code

    Args:
        status (int): HTTP status code
    """
    # pylint:disable=too-many-branches,too-many-return-statements
+    # See: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#status


I feel like this section could be refactored to a much simpler tree of conditions.

Perhaps. I left it in there just to show the possible httpstatuses.

...tation/opentelemetry-instrumentation-aiohttp-client/tests/test_aiohttp_client_integration.py

…try-python into status

remove canonical, change defaults

cf13537

lzchen force-pushed the status branch from 33b40a7 to cf13537 Compare October 26, 2020 22:40

lzchen added 15 commits October 26, 2020 18:45

api tests

846361b

temp tests

2fc1770

exporter

533b1f0

unknown

fc842d5

change default to unset, http_to_canon_code

0aceb53

test

6e55291

instrumentations

0659b75

Update tox.ini

72fa7fa

remove grpc codes

84251b5

grpc

fc06302

tests

85831fd

tests

2a4e87e

error

4bf4c66

do not set status on span end

28f0a12

tests

ec1c1a5

lzchen changed the title ~~Consistent way for error handling with status codes~~ Remove grpc status codes Oct 27, 2020

test

e4f25db

lzchen marked this pull request as ready for review October 27, 2020 21:26

lzchen requested a review from a team as a code owner October 27, 2020 21:26

lzchen requested review from codeboten and toumorokoshi and removed request for a team October 27, 2020 21:26

owais approved these changes Oct 28, 2020

View reviewed changes

toumorokoshi suggested changes Oct 28, 2020

View reviewed changes

lzchen mentioned this pull request Oct 28, 2020

Update OTLP/Opencensus exporter status mappings when proto is updated #1294

Closed

comment

1bb0309

lzchen changed the title ~~Remove grpc status codes~~ Change status codes from grpc status codes, remove setting status in instrumentations except on ERROR Oct 28, 2020

lzchen added 5 commits October 28, 2020 12:07

remove TODOs

2a56e0c

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

7da506d

…try-python into status

test

783569d

canonical_code -> status_code

d515597

Merge branch 'master' into status

aeeedef

toumorokoshi approved these changes Oct 28, 2020

View reviewed changes

lzchen added 2 commits October 28, 2020 17:04

lint

13ad8b4

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

d1a0b5f

…try-python into status

lzchen merged commit f3cdfa2 into open-telemetry:master Oct 28, 2020

lzchen deleted the status branch October 28, 2020 21:29

lzchen mentioned this pull request Oct 28, 2020

Have a consistent way of recording exception and status of span #1301

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change status codes from grpc status codes, remove setting status in instrumentations except on ERROR #1282

Change status codes from grpc status codes, remove setting status in instrumentations except on ERROR #1282

lzchen commented Oct 26, 2020 •

edited

Loading

owais left a comment

owais Oct 28, 2020

lzchen Oct 28, 2020

owais Oct 28, 2020

lzchen Oct 28, 2020

lzchen Oct 28, 2020

toumorokoshi left a comment

lzchen commented Oct 28, 2020

lzchen commented Oct 28, 2020

toumorokoshi left a comment

toumorokoshi Oct 28, 2020

lzchen Oct 28, 2020

toumorokoshi Oct 28, 2020

lzchen Oct 28, 2020

	# TODO: Remove setting status in instrumentation
	# TODO: Remove setting OK status in instrumentation

Change status codes from grpc status codes, remove setting status in instrumentations except on ERROR #1282

Change status codes from grpc status codes, remove setting status in instrumentations except on ERROR #1282

Conversation

lzchen commented Oct 26, 2020 • edited Loading

owais left a 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

lzchen commented Oct 28, 2020

lzchen commented Oct 28, 2020

toumorokoshi left a 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

lzchen commented Oct 26, 2020 •

edited

Loading