diff --git a/.flake8 b/.flake8 index 5abd0630ea..a0924f947d 100644 --- a/.flake8 +++ b/.flake8 @@ -10,6 +10,7 @@ exclude = .svn .tox CVS + target __pycache__ ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/gen/ ext/opentelemetry-ext-jaeger/build/* diff --git a/.gitignore b/.gitignore index 679b6fd0cc..c9acc31940 100644 --- a/.gitignore +++ b/.gitignore @@ -53,3 +53,4 @@ _build/ # mypy .mypy_cache/ +target diff --git a/.isort.cfg b/.isort.cfg index 4bf64a34f1..96011ae93d 100644 --- a/.isort.cfg +++ b/.isort.cfg @@ -12,4 +12,5 @@ line_length=79 ; ) ; docs: https://github.com/timothycrosley/isort#multi-line-output-modes multi_line_output=3 +skip=target skip_glob=ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/gen/* diff --git a/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py b/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py index a88782d642..e5dc9654fd 100644 --- a/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py +++ b/ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py @@ -210,7 +210,7 @@ def test_request_attributes_with_partial_raw_uri(self): self.validate_url("http://127.0.0.1/#top") def test_request_attributes_with_partial_raw_uri_and_nonstandard_port( - self + self, ): self.environ["RAW_URI"] = "/?" del self.environ["HTTP_HOST"] diff --git a/opentelemetry-api/src/opentelemetry/context/propagation/tracecontexthttptextformat.py b/opentelemetry-api/src/opentelemetry/context/propagation/tracecontexthttptextformat.py index abe778db95..20f2601fa2 100644 --- a/opentelemetry-api/src/opentelemetry/context/propagation/tracecontexthttptextformat.py +++ b/opentelemetry-api/src/opentelemetry/context/propagation/tracecontexthttptextformat.py @@ -39,11 +39,13 @@ ) _DELIMITER_FORMAT = "[ \t]*,[ \t]*" -_MEMBER_FORMAT = "({})(=)({})".format(_KEY_FORMAT, _VALUE_FORMAT) +_MEMBER_FORMAT = "({})(=)({})[ \t]*".format(_KEY_FORMAT, _VALUE_FORMAT) _DELIMITER_FORMAT_RE = re.compile(_DELIMITER_FORMAT) _MEMBER_FORMAT_RE = re.compile(_MEMBER_FORMAT) +_TRACECONTEXT_MAXIMUM_TRACESTATE_KEYS = 32 + class TraceContextHTTPTextFormat(httptextformat.HTTPTextFormat): """Extracts and injects using w3c TraceContext's headers. @@ -86,15 +88,10 @@ def extract( if version == "ff": return trace.INVALID_SPAN_CONTEXT - tracestate = trace.TraceState() - for tracestate_header in get_from_carrier( + tracestate_headers = get_from_carrier( carrier, cls._TRACESTATE_HEADER_NAME - ): - # typing.Dict's update is not recognized by pylint: - # https://github.com/PyCQA/pylint/issues/2420 - tracestate.update( # pylint:disable=E1101 - _parse_tracestate(tracestate_header) - ) + ) + tracestate = _parse_tracestate(tracestate_headers) span_context = trace.SpanContext( trace_id=int(trace_id, 16), @@ -127,8 +124,8 @@ def inject( ) -def _parse_tracestate(string: str) -> trace.TraceState: - """Parse a w3c tracestate header into a TraceState. +def _parse_tracestate(header_list: typing.List[str]) -> trace.TraceState: + """Parse one or more w3c tracestate header into a TraceState. Args: string: the value of the tracestate header. @@ -136,16 +133,35 @@ def _parse_tracestate(string: str) -> trace.TraceState: Returns: A valid TraceState that contains values extracted from the tracestate header. + + If the format of one headers is illegal, all values will + be discarded and an empty tracestate will be returned. + + If the number of keys is beyond the maximum, all values + will be discarded and an empty tracestate will be returned. """ tracestate = trace.TraceState() - for member in re.split(_DELIMITER_FORMAT_RE, string): - match = _MEMBER_FORMAT_RE.match(member) - if not match: - raise ValueError("illegal key-value format %r" % (member)) - key, _eq, value = match.groups() - # typing.Dict's update is not recognized by pylint: - # https://github.com/PyCQA/pylint/issues/2420 - tracestate[key] = value # pylint:disable=E1137 + value_count = 0 + for header in header_list: + for member in re.split(_DELIMITER_FORMAT_RE, header): + # empty members are valid, but no need to process further. + if not member: + continue + match = _MEMBER_FORMAT_RE.fullmatch(member) + if not match: + # TODO: log this? + return trace.TraceState() + key, _eq, value = match.groups() + if key in tracestate: # pylint:disable=E1135 + # duplicate keys are not legal in + # the header, so we will remove + return trace.TraceState() + # typing.Dict's update is not recognized by pylint: + # https://github.com/PyCQA/pylint/issues/2420 + tracestate[key] = value # pylint:disable=E1137 + value_count += 1 + if value_count > _TRACECONTEXT_MAXIMUM_TRACESTATE_KEYS: + return trace.TraceState() return tracestate diff --git a/opentelemetry-api/src/opentelemetry/distributedcontext/propagation/binaryformat.py b/opentelemetry-api/src/opentelemetry/distributedcontext/propagation/binaryformat.py index 15f8cfdf63..d6d083c0da 100644 --- a/opentelemetry-api/src/opentelemetry/distributedcontext/propagation/binaryformat.py +++ b/opentelemetry-api/src/opentelemetry/distributedcontext/propagation/binaryformat.py @@ -44,7 +44,7 @@ def to_bytes(context: DistributedContext) -> bytes: @staticmethod @abc.abstractmethod def from_bytes( - byte_representation: bytes + byte_representation: bytes, ) -> typing.Optional[DistributedContext]: """Return a DistributedContext that was represented by bytes. diff --git a/opentelemetry-api/src/opentelemetry/propagators/__init__.py b/opentelemetry-api/src/opentelemetry/propagators/__init__.py index 5b71e8785a..bb75d84c3a 100644 --- a/opentelemetry-api/src/opentelemetry/propagators/__init__.py +++ b/opentelemetry-api/src/opentelemetry/propagators/__init__.py @@ -78,7 +78,7 @@ def get_global_httptextformat() -> httptextformat.HTTPTextFormat: def set_global_httptextformat( - http_text_format: httptextformat.HTTPTextFormat + http_text_format: httptextformat.HTTPTextFormat, ) -> None: global _HTTP_TEXT_FORMAT # pylint:disable=global-statement _HTTP_TEXT_FORMAT = http_text_format diff --git a/opentelemetry-api/src/opentelemetry/trace/__init__.py b/opentelemetry-api/src/opentelemetry/trace/__init__.py index b64ce852b0..1ac7d73d49 100644 --- a/opentelemetry-api/src/opentelemetry/trace/__init__.py +++ b/opentelemetry-api/src/opentelemetry/trace/__init__.py @@ -338,10 +338,11 @@ def __init__( self.trace_state = trace_state def __repr__(self) -> str: - return "{}(trace_id={}, span_id={})".format( + return "{}(trace_id={}, span_id={}, trace_state={!r})".format( type(self).__name__, format_trace_id(self.trace_id), format_span_id(self.span_id), + self.trace_state, ) def is_valid(self) -> bool: @@ -589,7 +590,7 @@ def tracer() -> Tracer: def set_preferred_tracer_implementation( - factory: ImplementationFactory + factory: ImplementationFactory, ) -> None: """Set the factory to be used to create the tracer. diff --git a/opentelemetry-api/src/opentelemetry/util/loader.py b/opentelemetry-api/src/opentelemetry/util/loader.py index 0781ce15b7..3ae5a52fc5 100644 --- a/opentelemetry-api/src/opentelemetry/util/loader.py +++ b/opentelemetry-api/src/opentelemetry/util/loader.py @@ -173,7 +173,7 @@ def _load_impl( def set_preferred_default_implementation( - implementation_factory: _UntrustedImplFactory[_T] + implementation_factory: _UntrustedImplFactory[_T], ) -> None: """Sets a factory function that may be called for any implementation object. See the :ref:`module docs ` for more details.""" diff --git a/opentelemetry-api/tests/context/propagation/test_tracecontexthttptextformat.py b/opentelemetry-api/tests/context/propagation/test_tracecontexthttptextformat.py index aaf392be24..ed952e0dba 100644 --- a/opentelemetry-api/tests/context/propagation/test_tracecontexthttptextformat.py +++ b/opentelemetry-api/tests/context/propagation/test_tracecontexthttptextformat.py @@ -22,10 +22,10 @@ def get_as_list( - dict_object: typing.Dict[str, str], key: str + dict_object: typing.Dict[str, typing.List[str]], key: str ) -> typing.List[str]: value = dict_object.get(key) - return [value] if value is not None else [] + return value if value is not None else [] class TestTraceContextFormat(unittest.TestCase): @@ -40,64 +40,10 @@ def test_no_traceparent_header(self): If no traceparent header is received, the vendor creates a new trace-id and parent-id that represents the current request. """ - output = {} # type:typing.Dict[str, str] + output = {} # type:typing.Dict[str, typing.List[str]] span_context = FORMAT.extract(get_as_list, output) self.assertTrue(isinstance(span_context, trace.SpanContext)) - def test_from_headers_tracestate_entry_limit(self): - """If more than 33 entries are passed, allow them. - - We are explicitly choosing not to limit the list members - as outlined in RFC 3.3.1.1 - - RFC 3.3.1.1 - - There can be a maximum of 32 list-members in a list. - """ - - span_context = FORMAT.extract( - get_as_list, - { - "traceparent": "00-12345678901234567890123456789012-1234567890123456-00", - "tracestate": ",".join( - [ - "a00=0,a01=1,a02=2,a03=3,a04=4,a05=5,a06=6,a07=7,a08=8,a09=9", - "b00=0,b01=1,b02=2,b03=3,b04=4,b05=5,b06=6,b07=7,b08=8,b09=9", - "c00=0,c01=1,c02=2,c03=3,c04=4,c05=5,c06=6,c07=7,c08=8,c09=9", - "d00=0,d01=1,d02=2", - ] - ), - }, - ) - self.assertEqual(len(span_context.trace_state), 33) - - def test_from_headers_tracestate_duplicated_keys(self): - """If a duplicate tracestate header is present, the most recent entry - is used. - - RFC 3.3.1.4 - - Only one entry per key is allowed because the entry represents that last position in the trace. - Hence vendors must overwrite their entry upon reentry to their tracing system. - - For example, if a vendor name is Congo and a trace started in their system and then went through - a system named Rojo and later returned to Congo, the tracestate value would not be: - - congo=congosFirstPosition,rojo=rojosFirstPosition,congo=congosSecondPosition - - Instead, the entry would be rewritten to only include the most recent position: - - congo=congosSecondPosition,rojo=rojosFirstPosition - """ - span_context = FORMAT.extract( - get_as_list, - { - "traceparent": "00-12345678901234567890123456789012-1234567890123456-00", - "tracestate": "foo=1,bar=2,foo=3", - }, - ) - self.assertEqual(span_context.trace_state, {"foo": "3", "bar": "2"}) - def test_headers_with_tracestate(self): """When there is a traceparent and tracestate header, data from both should be addded to the SpanContext. @@ -109,7 +55,10 @@ def test_headers_with_tracestate(self): tracestate_value = "foo=1,bar=2,baz=3" span_context = FORMAT.extract( get_as_list, - {"traceparent": traceparent_value, "tracestate": tracestate_value}, + { + "traceparent": [traceparent_value], + "tracestate": [tracestate_value], + }, ) self.assertEqual(span_context.trace_id, self.TRACE_ID) self.assertEqual(span_context.span_id, self.SPAN_ID) @@ -125,7 +74,8 @@ def test_headers_with_tracestate(self): self.assertEqual(output["tracestate"].count(","), 2) def test_invalid_trace_id(self): - """If the trace id is invalid, we must ignore the full traceparent header. + """If the trace id is invalid, we must ignore the full traceparent header, + and return a random, valid trace. Also ignore any tracestate. @@ -142,8 +92,10 @@ def test_invalid_trace_id(self): span_context = FORMAT.extract( get_as_list, { - "traceparent": "00-00000000000000000000000000000000-1234567890123456-00", - "tracestate": "foo=1,bar=2,foo=3", + "traceparent": [ + "00-00000000000000000000000000000000-1234567890123456-00" + ], + "tracestate": ["foo=1,bar=2,foo=3"], }, ) self.assertEqual(span_context, trace.INVALID_SPAN_CONTEXT) @@ -166,8 +118,10 @@ def test_invalid_parent_id(self): span_context = FORMAT.extract( get_as_list, { - "traceparent": "00-00000000000000000000000000000000-0000000000000000-00", - "tracestate": "foo=1,bar=2,foo=3", + "traceparent": [ + "00-00000000000000000000000000000000-0000000000000000-00" + ], + "tracestate": ["foo=1,bar=2,foo=3"], }, ) self.assertEqual(span_context, trace.INVALID_SPAN_CONTEXT) @@ -195,14 +149,15 @@ def test_format_not_supported(self): RFC 4.3 - If the version cannot be parsed, the vendor creates a new traceparent header and - deletes tracestate. + If the version cannot be parsed, return an invalid trace header. """ span_context = FORMAT.extract( get_as_list, { - "traceparent": "00-12345678901234567890123456789012-1234567890123456-00-residue", - "tracestate": "foo=1,bar=2,foo=3", + "traceparent": [ + "00-12345678901234567890123456789012-1234567890123456-00-residue" + ], + "tracestate": ["foo=1,bar=2,foo=3"], }, ) self.assertEqual(span_context, trace.INVALID_SPAN_CONTEXT) @@ -213,3 +168,30 @@ def test_propagate_invalid_context(self): output = {} # type:typing.Dict[str, str] FORMAT.inject(trace.INVALID_SPAN_CONTEXT, dict.__setitem__, output) self.assertFalse("traceparent" in output) + + def test_tracestate_empty_header(self): + """Test tracestate with an additional empty header (should be ignored)""" + span_context = FORMAT.extract( + get_as_list, + { + "traceparent": [ + "00-12345678901234567890123456789012-1234567890123456-00" + ], + "tracestate": ["foo=1", ""], + }, + ) + self.assertEqual(span_context.trace_state["foo"], "1") + + def test_tracestate_header_with_trailing_comma(self): + """Do not propagate invalid trace context. + """ + span_context = FORMAT.extract( + get_as_list, + { + "traceparent": [ + "00-12345678901234567890123456789012-1234567890123456-00" + ], + "tracestate": ["foo=1,"], + }, + ) + self.assertEqual(span_context.trace_state["foo"], "1") diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 879d4e6385..0909b9b434 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -301,7 +301,6 @@ def set_status(self, status: trace_api.Status) -> None: def generate_span_id() -> int: """Get a new random span ID. - Returns: A random 64-bit int for use as a span ID """ @@ -310,7 +309,6 @@ def generate_span_id() -> int: def generate_trace_id() -> int: """Get a new random trace ID. - Returns: A random 128-bit int for use as a trace ID """ diff --git a/opentelemetry-sdk/tests/context/propagation/test_b3_format.py b/opentelemetry-sdk/tests/context/propagation/test_b3_format.py index 09d3f88f41..1215508269 100644 --- a/opentelemetry-sdk/tests/context/propagation/test_b3_format.py +++ b/opentelemetry-sdk/tests/context/propagation/test_b3_format.py @@ -16,7 +16,7 @@ import opentelemetry.sdk.context.propagation.b3_format as b3_format import opentelemetry.sdk.trace as trace -import opentelemetry.trace as api_trace +import opentelemetry.trace as trace_api FORMAT = b3_format.B3Format() @@ -163,8 +163,8 @@ def test_invalid_single_header(self): """ carrier = {FORMAT.SINGLE_HEADER_KEY: "0-1-2-3-4-5-6-7"} span_context = FORMAT.extract(get_as_list, carrier) - self.assertEqual(span_context.trace_id, api_trace.INVALID_TRACE_ID) - self.assertEqual(span_context.span_id, api_trace.INVALID_SPAN_ID) + self.assertEqual(span_context.trace_id, trace_api.INVALID_TRACE_ID) + self.assertEqual(span_context.span_id, trace_api.INVALID_SPAN_ID) def test_missing_trace_id(self): """If a trace id is missing, populate an invalid trace id.""" @@ -173,7 +173,7 @@ def test_missing_trace_id(self): FORMAT.FLAGS_KEY: "1", } span_context = FORMAT.extract(get_as_list, carrier) - self.assertEqual(span_context.trace_id, api_trace.INVALID_TRACE_ID) + self.assertEqual(span_context.trace_id, trace_api.INVALID_TRACE_ID) def test_missing_span_id(self): """If a trace id is missing, populate an invalid trace id.""" @@ -182,4 +182,4 @@ def test_missing_span_id(self): FORMAT.FLAGS_KEY: "1", } span_context = FORMAT.extract(get_as_list, carrier) - self.assertEqual(span_context.span_id, api_trace.INVALID_SPAN_ID) + self.assertEqual(span_context.span_id, trace_api.INVALID_SPAN_ID) diff --git a/pyproject.toml b/pyproject.toml index eff7e2e3ec..cb3c2eb546 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -9,8 +9,10 @@ exclude = ''' | \.mypy_cache | \.tox | \.venv + | \.vscode | _build | buck-out + | target | build | dist | ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/gen # generated files diff --git a/scripts/tracecontext-integration-test.sh b/scripts/tracecontext-integration-test.sh new file mode 100755 index 0000000000..4d482ddafe --- /dev/null +++ b/scripts/tracecontext-integration-test.sh @@ -0,0 +1,27 @@ +#!/bin/sh +set -e +# hard-coding the git tag to ensure stable builds. +TRACECONTEXT_GIT_TAG="98f210efd89c63593dce90e2bae0a1bdcb986f51" +# clone w3c tracecontext tests +mkdir -p target +rm -rf ./target/trace-context +git clone https://github.com/w3c/trace-context ./target/trace-context +cd ./target/trace-context && git checkout $TRACECONTEXT_GIT_TAG && cd - +# start example opentelemetry service, which propagates trace-context by +# default. +python ./tests/w3c_tracecontext_validation_server.py 1>&2 & +EXAMPLE_SERVER_PID=$! +# give the app server a little time to start up. Not adding some sort +# of delay would cause many of the tracecontext tests to fail being +# unable to connect. +sleep 1 +onshutdown() +{ + # send a sigint, to ensure + # it is caught as a KeyboardInterrupt in the + # example service. + kill $EXAMPLE_SERVER_PID +} +trap onshutdown EXIT +cd ./target/trace-context/test +python test.py http://127.0.0.1:5000/verify-tracecontext diff --git a/tests/w3c_tracecontext_validation_server.py b/tests/w3c_tracecontext_validation_server.py new file mode 100644 index 0000000000..a26141f14c --- /dev/null +++ b/tests/w3c_tracecontext_validation_server.py @@ -0,0 +1,77 @@ +#!/usr/bin/env python3 +# +# Copyright 2019, OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +""" +This server is intended to be used with the W3C tracecontext validation +Service. It implements the APIs needed to be exercised by the test bed. +""" + +import json + +import flask +import requests + +from opentelemetry import trace +from opentelemetry.ext import http_requests +from opentelemetry.ext.wsgi import OpenTelemetryMiddleware +from opentelemetry.sdk.trace import Tracer +from opentelemetry.sdk.trace.export import ( + ConsoleSpanExporter, + SimpleExportSpanProcessor, +) + +# The preferred tracer implementation must be set, as the opentelemetry-api +# defines the interface with a no-op implementation. +trace.set_preferred_tracer_implementation(lambda T: Tracer()) + +# Integrations are the glue that binds the OpenTelemetry API and the +# frameworks and libraries that are used together, automatically creating +# Spans and propagating context as appropriate. +http_requests.enable(trace.tracer()) + +# SpanExporter receives the spans and send them to the target location. +span_processor = SimpleExportSpanProcessor(ConsoleSpanExporter()) +trace.tracer().add_span_processor(span_processor) + +app = flask.Flask(__name__) +app.wsgi_app = OpenTelemetryMiddleware(app.wsgi_app) + + +@app.route("/verify-tracecontext", methods=["POST"]) +def verify_tracecontext(): + """Upon reception of some payload, sends a request back to the designated + url. + + This route is designed to be testable with the w3c tracecontext server / + client test. + """ + for action in flask.request.json: + requests.post( + url=action["url"], + data=json.dumps(action["arguments"]), + headers={ + "Accept": "application/json", + "Content-Type": "application/json; charset=utf-8", + }, + timeout=5.0, + ) + return "hello" + + +if __name__ == "__main__": + try: + app.run(debug=True) + finally: + span_processor.shutdown() diff --git a/tox.ini b/tox.ini index a6abe8e158..e30cb1a14b 100644 --- a/tox.ini +++ b/tox.ini @@ -5,6 +5,7 @@ envlist = py3{4,5,6,7,8}-test-{api,sdk,example-app,ext-wsgi,ext-http-requests,ext-jaeger,opentracing-shim} pypy3-test-{api,sdk,example-app,ext-wsgi,ext-http-requests,ext-jaeger,opentracing-shim} lint + py37-tracecontext py37-{mypy,mypyinstalled} docs @@ -111,3 +112,21 @@ changedir = docs commands = sphinx-build -W --keep-going -b html -T . _build/html + +[testenv:py37-tracecontext] +basepython: python3.7 +deps = + # needed for tracecontext + aiohttp~=3.6 + # needed for example trace integration + flask~=1.1 + requests~=2.7 + +commands_pre = + pip install -e {toxinidir}/opentelemetry-api + pip install -e {toxinidir}/opentelemetry-sdk + pip install -e {toxinidir}/ext/opentelemetry-ext-http-requests + pip install -e {toxinidir}/ext/opentelemetry-ext-wsgi + +commands = + {toxinidir}/scripts/tracecontext-integration-test.sh