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

Add NoOpTracerProvider test cases for sqllite3 instrumentation #2709

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Sunilwali679
Copy link

@Sunilwali679 Sunilwali679 commented Jul 16, 2024

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.

Added test case for test_no_op_tracer_provider
updated the len(spans)
Copy link

linux-foundation-easycla bot commented Jul 16, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@xrmx
Copy link
Contributor

xrmx commented Jul 17, 2024

@Sunilwali679 you have to sign the CLA in order to contribute to this project, thanks

@xrmx xrmx marked this pull request as draft July 17, 2024 07:27
@xrmx xrmx changed the title Opentelemetry instrumentation sqlite3 964 Add NoOpTracerProvider test cases for sqllite3 instrumentation Jul 17, 2024
@Sunilwali679 Sunilwali679 force-pushed the opentelemetry-instrumentation-sqlite3-964 branch from 8a37f52 to ca72364 Compare July 24, 2024 11:20
@Sunilwali679 Sunilwali679 marked this pull request as ready for review July 24, 2024 11:20
@Sunilwali679
Copy link
Author

@xrmx could you please review the PR ? I have rebased and updated CLA

@emdneto emdneto requested a review from a team July 24, 2024 11:46
@emdneto emdneto added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 24, 2024
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)
Copy link
Contributor

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?

Copy link
Member

@emdneto emdneto Jul 24, 2024

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

Copy link
Author

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 ?

Copy link
Member

@emdneto emdneto Jul 24, 2024

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

Copy link
Author

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)

Copy link
Author

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 .

Copy link
Member

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

Copy link
Author

Choose a reason for hiding this comment

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

@xrmx @emdneto ,

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%]

Copy link
Author

Choose a reason for hiding this comment

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

@xrmx @emdneto . could you please review and approve the PR

thanks

Signed-off-by: Wali, Sunil Shidrayi <SunilShidrayi.Wali@fmr.com>
@brianwarner brianwarner force-pushed the opentelemetry-instrumentation-sqlite3-964 branch from ca72364 to c78e4a3 Compare July 29, 2024 16:37
Copy link
Member

@emdneto emdneto left a 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?

@xrmx
Copy link
Contributor

xrmx commented Aug 1, 2024

CI is failing. Could you please double-check it?

Is it me or test asserts have been removed?

@emdneto
Copy link
Member

emdneto commented Aug 1, 2024

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.

@emdneto emdneto marked this pull request as draft August 1, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Audit and test opentelemetry-instrumentation-sqlite3
3 participants