Skip to content
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.

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

Improve reliability of tests #643

Merged
merged 11 commits into from
Sep 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ on:
- 'release/*'
pull_request:
env:
CORE_REPO_SHA: 9dc17e33bc083e38c1fd55b8ed6787caba8f54fe
CORE_REPO_SHA: c49ad57bfe35cfc69bfa863d74058ca9bec55fc3

jobs:
build:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,14 @@

import itertools
import logging
import sys
import time
import unittest
from unittest import mock

from ddtrace.internal.writer import AgentWriter
from flaky import flaky
from pytest import mark

from opentelemetry import trace as trace_api
from opentelemetry.context import Context
Expand Down Expand Up @@ -469,6 +471,9 @@ def test_span_processor_dropped_spans(self):
self.assertEqual(len(datadog_spans), 128)
tracer_provider.shutdown()

@mark.skipif(
sys.platform == "win32", reason="unreliable test on windows",
)
def test_span_processor_scheduled_delay(self):
"""Test that spans are exported each schedule_delay_millis"""
delay = 300
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.
import io
import json
import sys
import zipfile
from unittest.mock import Mock, patch

Expand All @@ -30,6 +31,7 @@
mock_sts,
mock_xray,
)
from pytest import mark

from opentelemetry import trace as trace_api
from opentelemetry.context import attach, detach, set_value
Expand Down Expand Up @@ -128,12 +130,13 @@ def test_s3_client(self):
s3.list_buckets()
s3.list_buckets()

spans = self.memory_exporter.get_finished_spans()
spans = self.get_finished_spans()
assert spans
span = spans[0]
self.assertEqual(len(spans), 2)

buckets_span = spans.by_attr("aws.operation", "ListBuckets")
self.assertSpanHasAttributes(
span,
buckets_span,
{
"aws.operation": "ListBuckets",
"aws.region": "us-west-2",
Expand All @@ -144,22 +147,21 @@ def test_s3_client(self):
)

# testing for span error
self.memory_exporter.get_finished_spans()
with self.assertRaises(ParamValidationError):
s3.list_objects(bucket="mybucket")
spans = self.memory_exporter.get_finished_spans()
spans = self.get_finished_spans()
assert spans
span = spans[2]
objects_span = spans.by_attr("aws.operation", "ListObjects")
self.assertSpanHasAttributes(
span,
objects_span,
{
"aws.operation": "ListObjects",
"aws.region": "us-west-2",
"aws.service": "s3",
},
)
self.assertIs(
span.status.status_code, trace_api.StatusCode.ERROR,
objects_span.status.status_code, trace_api.StatusCode.ERROR,
)

# Comment test for issue 1088
Expand All @@ -172,10 +174,11 @@ def test_s3_put(self):
s3.put_object(**params)
s3.get_object(Bucket="mybucket", Key="foo")

spans = self.memory_exporter.get_finished_spans()
spans = self.get_finished_spans()
assert spans
self.assertEqual(len(spans), 3)
create_span = spans[0]

create_span = spans.by_attr("aws.operation", "CreateBucket")
self.assertSpanHasAttributes(
create_span,
{
Expand All @@ -186,7 +189,8 @@ def test_s3_put(self):
SpanAttributes.HTTP_STATUS_CODE: 200,
},
)
put_span = spans[1]

put_span = spans.by_attr("aws.operation", "PutObject")
self.assertSpanHasAttributes(
put_span,
{
Expand All @@ -197,8 +201,10 @@ def test_s3_put(self):
SpanAttributes.HTTP_STATUS_CODE: 200,
},
)
self.assertTrue("params.Body" not in spans[1].attributes.keys())
get_span = spans[2]
self.assertTrue("params.Body" not in put_span.attributes.keys())

get_span = spans.by_attr("aws.operation", "GetObject")

self.assertSpanHasAttributes(
get_span,
{
Expand Down Expand Up @@ -359,6 +365,10 @@ def get_role_name(self):
Path="/my-path/",
)["Role"]["Arn"]

@mark.skipif(
sys.platform == "win32",
reason="requires docker and Github CI Windows does not have docker installed by default",
)
@mock_lambda
def test_lambda_invoke_propagation(self):

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,19 +60,13 @@ def tearDown(self):
with self.disable_logging():
ElasticsearchInstrumentor().uninstrument()

def get_ordered_finished_spans(self):
return sorted(
self.memory_exporter.get_finished_spans(),
key=lambda s: s.start_time,
)

def test_instrumentor(self, request_mock):
request_mock.return_value = (1, {}, {})

es = Elasticsearch()
es.index(index="sw", doc_type="people", id=1, body={"name": "adam"})

spans_list = self.get_ordered_finished_spans()
spans_list = self.get_finished_spans()
self.assertEqual(len(spans_list), 1)
span = spans_list[0]

Expand All @@ -87,7 +81,7 @@ def test_instrumentor(self, request_mock):

es.index(index="sw", doc_type="people", id=1, body={"name": "adam"})

spans_list = self.get_ordered_finished_spans()
spans_list = self.get_finished_spans()
self.assertEqual(len(spans_list), 1)

def test_span_not_recording(self, request_mock):
Expand Down Expand Up @@ -127,7 +121,7 @@ def _test_prefix(self, prefix):
es = Elasticsearch()
es.index(index="sw", doc_type="people", id=1, body={"name": "adam"})

spans_list = self.get_ordered_finished_spans()
spans_list = self.get_finished_spans()
self.assertEqual(len(spans_list), 1)
span = spans_list[0]
self.assertTrue(span.name.startswith(prefix))
Expand All @@ -141,7 +135,7 @@ def test_result_values(self, request_mock):
es = Elasticsearch()
es.get(index="test-index", doc_type="tweet", id=1)

spans = self.get_ordered_finished_spans()
spans = self.get_finished_spans()

self.assertEqual(1, len(spans))
self.assertEqual("False", spans[0].attributes["elasticsearch.found"])
Expand Down Expand Up @@ -169,7 +163,7 @@ def _test_trace_error(self, code, exc):
except Exception: # pylint: disable=broad-except
pass

spans = self.get_ordered_finished_spans()
spans = self.get_finished_spans()
self.assertEqual(1, len(spans))
span = spans[0]
self.assertFalse(span.status.is_ok)
Expand All @@ -186,13 +180,13 @@ def test_parent(self, request_mock):
index="sw", doc_type="people", id=1, body={"name": "adam"}
)

spans = self.get_ordered_finished_spans()
spans = self.get_finished_spans()
self.assertEqual(len(spans), 2)

self.assertEqual(spans[0].name, "parent")
self.assertEqual(spans[1].name, "Elasticsearch/sw/people/1")
self.assertIsNotNone(spans[1].parent)
self.assertEqual(spans[1].parent.span_id, spans[0].context.span_id)
parent = spans.by_name("parent")
child = spans.by_name("Elasticsearch/sw/people/1")
self.assertIsNotNone(child.parent)
self.assertEqual(child.parent.span_id, parent.context.span_id)

def test_multithread(self, request_mock):
request_mock.return_value = (1, {}, {})
Expand Down Expand Up @@ -222,16 +216,15 @@ def target2():
t1.join()
t2.join()

spans = self.get_ordered_finished_spans()
spans = self.get_finished_spans()
self.assertEqual(3, len(spans))
s1, s2, s3 = spans

self.assertEqual(s1.name, "parent")
s1 = spans.by_name("parent")
s2 = spans.by_name("Elasticsearch/test-index/tweet/1")
s3 = spans.by_name("Elasticsearch/test-index/tweet/2")

self.assertEqual(s2.name, "Elasticsearch/test-index/tweet/1")
self.assertIsNotNone(s2.parent)
self.assertEqual(s2.parent.span_id, s1.context.span_id)
self.assertEqual(s3.name, "Elasticsearch/test-index/tweet/2")
self.assertIsNone(s3.parent)

def test_dsl_search(self, request_mock):
Expand All @@ -242,7 +235,7 @@ def test_dsl_search(self, request_mock):
"term", author="testing"
)
search.execute()
spans = self.get_ordered_finished_spans()
spans = self.get_finished_spans()
span = spans[0]
self.assertEqual(1, len(spans))
self.assertEqual(span.name, "Elasticsearch/test-index/_search")
Expand Down Expand Up @@ -270,10 +263,11 @@ def test_dsl_create(self, request_mock):
client = Elasticsearch()
Article.init(using=client)

spans = self.get_ordered_finished_spans()
spans = self.get_finished_spans()
self.assertEqual(2, len(spans))
span1, span2 = spans
self.assertEqual(span1.name, "Elasticsearch/test-index")
Copy link
Member

Choose a reason for hiding this comment

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

Why delete this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not asserted by the span.by_name() call. It returns a span if found, otherwise throws an assertion error. Perhaps I should rename it to by_name_or_fail() or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LMK if that makes sense. I can follow up with another PR.

span1 = spans.by_attr(key="elasticsearch.method", value="HEAD")
span2 = spans.by_attr(key="elasticsearch.method", value="PUT")

self.assertEqual(
span1.attributes,
{
Expand All @@ -283,7 +277,6 @@ def test_dsl_create(self, request_mock):
},
)

self.assertEqual(span2.name, "Elasticsearch/test-index")
attributes = {
SpanAttributes.DB_SYSTEM: "elasticsearch",
"elasticsearch.url": "/test-index",
Expand All @@ -306,7 +299,7 @@ def test_dsl_index(self, request_mock):
)
res = article.save(using=client)
self.assertTrue(res)
spans = self.get_ordered_finished_spans()
spans = self.get_finished_spans()
self.assertEqual(1, len(spans))
span = spans[0]
self.assertEqual(span.name, helpers.dsl_index_span_name)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,15 +185,19 @@ def test_coroutine_handler(self):
def _test_async_handler(self, url, handler_name):
response = self.fetch(url)
self.assertEqual(response.code, 201)
spans = self.memory_exporter.get_finished_spans()
spans = self.get_finished_spans()
self.assertEqual(len(spans), 5)

sub2, sub1, sub_wrapper, server, client = self.sorted_spans(spans)
client = spans.by_name("GET")
server = spans.by_name(handler_name + ".get")
sub_wrapper = spans.by_name("sub-task-wrapper")

sub2 = spans.by_name("sub-task-2")
self.assertEqual(sub2.name, "sub-task-2")
self.assertEqual(sub2.parent, sub_wrapper.context)
self.assertEqual(sub2.context.trace_id, client.context.trace_id)

sub1 = spans.by_name("sub-task-1")
self.assertEqual(sub1.name, "sub-task-1")
self.assertEqual(sub1.parent, sub_wrapper.context)
self.assertEqual(sub1.context.trace_id, client.context.trace_id)
Expand Down Expand Up @@ -238,9 +242,11 @@ def test_500(self):
response = self.fetch("/error")
self.assertEqual(response.code, 500)

spans = self.sorted_spans(self.memory_exporter.get_finished_spans())
spans = self.get_finished_spans()
self.assertEqual(len(spans), 2)
server, client = spans

client = spans.by_name("GET")
server = spans.by_name("BadHandler.get")

self.assertEqual(server.name, "BadHandler.get")
self.assertEqual(server.kind, SpanKind.SERVER)
Expand Down
3 changes: 2 additions & 1 deletion tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,8 @@ envlist =
py3{6,7,8,9}-test-instrumentation-grpc

; opentelemetry-instrumentation-sqlalchemy
py3{6,7,8,9}-test-instrumentation-sqlalchemy{11,14}
py3{6,7}-test-instrumentation-sqlalchemy{11}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment explaining why we do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

py3{6,7,8,9}-test-instrumentation-sqlalchemy{14}
pypy3-test-instrumentation-sqlalchemy{11,14}

; opentelemetry-instrumentation-redis
Expand Down