-
Notifications
You must be signed in to change notification settings - Fork 628
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
Add NoOpTracerProvider test cases for sqllite3 instrumentation #2709
base: main
Are you sure you want to change the base?
Add NoOpTracerProvider test cases for sqllite3 instrumentation #2709
Conversation
Added test case for test_no_op_tracer_provider
updated the len(spans)
@Sunilwali679 you have to sign the CLA in order to contribute to this project, thanks |
8a37f52
to
ca72364
Compare
@xrmx could you please review the PR ? I have rebased and updated CLA |
with self._tracer.start_as_current_span("rootSpan"): | ||
self._cursor.executemany(stmt, data) | ||
spans = self.memory_exporter.get_finished_spans() | ||
self.assertEqual(len(spans), 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this supposed to be 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We should assert 0 spans
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xrmx
@emdneto
I am not finding the steps validate the testcases of the instrumentation
using tox - e earlier used to see here https://github.com/open-telemetry/opentelemetry-python-contrib
could you please help me to get those steps ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YOu can use tox -e py312-test-instrumentation-sqllite3. Use tox list
to see all possible combinations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @xrmx @emdneto
I tested for self.assertEqual(len(spans), 0) its failed
AssertionError: 2 != 0
instrumentation/opentelemetry-instrumentation-sqlite3/tests/test_sqlite3.py:115: AssertionError
as shown below
def test_noop_tracer_provider(self):
"""Should not create any spans when using NoOpTracerProvider"""
SQLite3Instrumentor().uninstrument()
SQLite3Instrumentor().instrument(tracer_provider=trace_api.get_tracer_provider())
self._connection = sqlite3.connect(":memory:")
self._cursor = self._connection.cursor()
stmt = "CREATE TABLE IF NOT EXISTS test (id integer)"
with self._tracer.start_as_current_span("rootSpan"):
self._cursor.execute(stmt)
spans = self.memory_exporter.get_finished_spans()
self.assertEqual(len(spans), 0)
self._create_tables()
stmt = "INSERT INTO test (id) VALUES (?)"
data = [("1",), ("2",), ("3",)]
with self._tracer.start_as_current_span("rootSpan"):
self._cursor.executemany(stmt, data)
spans = self.memory_exporter.get_finished_spans()
self.assertEqual(len(spans), 0)
with self._tracer.start_as_current_span("rootSpan"), self.assertRaises(Exception):
self._cursor.callproc("test", ())
spans = self.memory_exporter.get_finished_spans()
self.assertEqual(len(spans), 0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xrmx @xrmx i do see the other testcase validate_spans in the same file where length of the span validated by 2 self.assertEqual(len(spans), 2) one for root and other for child .
def validate_spans(self, span_name):
spans = self.memory_exporter.get_finished_spans()
self.memory_exporter.clear()
self.assertEqual(len(spans), 2)
for span in spans:
if span.name == "rootSpan":
root_span = span
else:
child_span = span
self.assertIsInstance(span.start_time, int)
self.assertIsInstance(span.end_time, int)
self.assertIsNotNone(root_span)
self.assertIsNotNone(child_span)
self.assertEqual(root_span.name, "rootSpan")
self.assertEqual(child_span.name, span_name)
self.assertIsNotNone(child_span.parent)
self.assertIs(child_span.parent, root_span.get_span_context())
self.assertIs(child_span.kind, trace_api.SpanKind.CLIENT)
I see length of span is 2 so assertion is done by 2 in the PR .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is noop tracer provider shouldn't produce any outcome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have merged with latest code with noop tracer provider shouldn't produce any outcome . I ran the tox testcase its passed 100%.
platform linux -- Python 3.10.12, pytest-7.4.4, pluggy-1.5.0 -- /.tox/py310-test-instrumentation-sqlite3/bin/python
cachedir: .tox/py310-test-instrumentation-sqlite3/.pytest_cache
rootdir: opentelemetry-python-contrib
configfile: pytest.ini
collected 4 items
instrumentation/opentelemetry-instrumentation-sqlite3/tests/test_sqlite3.py::TestSQLite3::test_callproc PASSED [ 25%]
instrumentation/opentelemetry-instrumentation-sqlite3/tests/test_sqlite3.py::TestSQLite3::test_execute PASSED [ 50%]
instrumentation/opentelemetry-instrumentation-sqlite3/tests/test_sqlite3.py::TestSQLite3::test_executemany PASSED [ 75%]
instrumentation/opentelemetry-instrumentation-sqlite3/tests/test_sqlite3.py::TestSQLite3::test_noop_tracer_provider PASSED [100%]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Wali, Sunil Shidrayi <[email protected]>
ca72364
to
c78e4a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is failing. Could you please double-check it?
Is it me or test asserts have been removed? |
Yes. @Sunilwali679 I think here you should double-check the test implementation and assert 0 spans. Marking as a draft to avoid wrong merge. |
Description
Adds NoOpTracerProvider test cases for sqllite3 instrumentation.
Fixes #964
How Has This Been Tested?
tox -e test-instrumentation-sqlite3
instrumentation/opentelemetry-instrumentation-sqlite3/tests/test_sqlite3.py::TestSQLite3::test_callproc PASSED [ 25%]
instrumentation/opentelemetry-instrumentation-sqlite3/tests/test_sqlite3.py::TestSQLite3::test_execute PASSED [ 50%]
instrumentation/opentelemetry-instrumentation-sqlite3/tests/test_sqlite3.py::TestSQLite3::test_executemany PASSED [ 75%]
instrumentation/opentelemetry-instrumentation-sqlite3/tests/test_sqlite3.py::TestSQLite3::test_no_op_tracer_provider PASSED [100%]
Does This PR Require a Core Repo Change?
No.