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

Audit and test opentelemetry-instrumentation-elasticsearch NoOpTracer… #1616

Conversation

Akochavi
Copy link
Contributor

@Akochavi Akochavi commented Feb 1, 2023

Add test for elasticsearch using NoOpTracerProvider

Fixes #984

How has this been tested?

tox -e test-instrumentation-elasticsearch

@Akochavi Akochavi requested a review from a team February 1, 2023 11:08
'{"found": false, "timed_out": true, "took": 7}',
)
es = Elasticsearch()
es.get(index="test-index", doc_type="_doc", id=1)
Copy link
Member

Choose a reason for hiding this comment

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

I suggest validating es.get response value -> so we'll know for sure that no spans were created due to NoOpTracerProvider

@Akochavi Akochavi force-pushed the audit-and-test-opentelemetry-instrumentation-elasticsearch-no-op-tracer branch from b0afd30 to 4be4c43 Compare February 5, 2023 07:06
@shalevr shalevr added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Feb 5, 2023
@@ -24,6 +24,7 @@

import opentelemetry.instrumentation.elasticsearch
from opentelemetry import trace
from opentelemetry import trace as trace_api
Copy link
Member

Choose a reason for hiding this comment

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

trace already imported in the upper line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -37,7 +37,7 @@

major_version = elasticsearch.VERSION[0]

if major_version == 7:
if major_version 7:
Copy link
Member

Choose a reason for hiding this comment

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

Is this a mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, I don't know who pushed this change but I will revert it

@Akochavi Akochavi force-pushed the audit-and-test-opentelemetry-instrumentation-elasticsearch-no-op-tracer branch from ffed02e to 34475e1 Compare February 15, 2023 11:57
@srikanthccv srikanthccv enabled auto-merge (squash) February 18, 2023 01:36
@srikanthccv srikanthccv merged commit aa6397a into open-telemetry:main Feb 18, 2023
shalevr added a commit to shalevr/opentelemetry-python-contrib that referenced this pull request Feb 23, 2023
…/github.com/shalevr/opentelemetry-python-contrib into Change-metrics-tests-to-work-with-test_base

* 'Change-metrics-tests-to-work-with-test_base' of https://github.com/shalevr/opentelemetry-python-contrib:
  Fix issue with Flask instrumentation when a request spawn children threads and copies the request context (open-telemetry#1654)
  Add connection attributes to sqlalchemy connect span (open-telemetry#1608)
  Add boto3sqs to docs (open-telemetry#1666)
  Audit and test opentelemetry-instrumentation-elasticsearch NoOpTracer… (open-telemetry#1616)
  Copy change log updates from release/v1.16.x-0.37bx (open-telemetry#1683)
  Update version to 1.17.0.dev/0.38b0.dev (open-telemetry#1677)
  Fix CI Failure (open-telemetry#1680)
  Add better debugging if hatch subprocess fails (open-telemetry#1672)
  Add confluent kafka docs (open-telemetry#1668)
  Support aio_pika 9 (open-telemetry#1670)
  Audit and test opentelemetry-instrumentation-wsgi NoOpTracerProvider (open-telemetry#1610)
  bot (open-telemetry#1667)
  Add commit method for ConfluentKafkaInstrumentor's ProxiedConsumer (open-telemetry#1656)
  Revert open-telemetry#1097 (open-telemetry#1660)
  Audit and test opentelemetry-instrumentation-django NoOpTracerProvider (open-telemetry#1611)
  Audit and test opentelemetry-instrumentation-aiohttp-client NoOpTrace… (open-telemetry#1612)
  Audit and test opentelemetry-instrumentation-flask NoOpTracerProvider (open-telemetry#1614)
  Audit and test opentelemetry-instrumentation-dbapi NoOpTracerProvider (open-telemetry#1607)
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-elasticsearch
4 participants