ref: Move OTel setup out of integrations/opentelemetry/
#4277
Conversation
…o ivana/potel/move-propagator
…o ivana/potel/move-propagator
…-scope-out-of-integrations
sentrivana
changed the title
opentelemetry/integrations/integrations/opentelemetry/
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## potel-base #4277 +/- ##
===========================================
Coverage 83.54% 83.55%
===========================================
Files 144 144
Lines 14631 14613 -18
Branches 2325 2324 -1
===========================================
- Hits 12224 12210 -14
+ Misses 1690 1686 -4
Partials 717 717
|
sentrivana
changed the title
integrations/opentelemetry/integrations/opentelemetry/
sentry_sdk/client.py
Outdated
| @@ -392,6 +393,13 @@ def _capture_envelope(envelope): | |||
| except Exception as e: | |||
| logger.debug("Can not set up continuous profiler. (%s)", e) | |||
|
|
|||
| from sentry_sdk.opentelemetry.integration import ( | |||
There was a problem hiding this comment.
This inline import is not great but having this on the top level causes Sphinx to fail and I've already spent too much time trying to fix this. (Ideas welcome)
There was a problem hiding this comment.
I have two possible ideas:
- Import the
sentry_sdk.opentelemetry.integrationmodule at the top, then call the methods on the module - Move the import to the bottom of the file in a "circular imports" section (I believe we have used this pattern elsewhere before, so it is likely preferable to an inline import if possible to do)
There was a problem hiding this comment.
Neither of those works, unfortunately. They lead to more Sphinx errors.
There was a problem hiding this comment.
[suggestion] We should call this file something other than integration.py, since Opentelemetry is part of the SDK now, and no longer an integration.
There was a problem hiding this comment.
Suggestions welcome -- I think going with 'integration' would still be ok since we're integrating with OTel in that file but I agree it can be confusing to folks.
There was a problem hiding this comment.
Agreed naming is hard – maybe we can call it setup or something like that, given that it sets up the link with Otel?
I just think integration is particularly confusing here. While it is true that we "integrate" with Opentelemetry, the terminology is confusing because in the SDK codebase, "integration" usually means an optional integration with a library like FastAPI, Celery, etc.
There was a problem hiding this comment.
I also considered setup but having a setup.py file is also not great 🫠
There was a problem hiding this comment.
Called it tracing.py now since it sets up OTel tracing
sentry_sdk/client.py
Outdated
| @@ -392,6 +393,13 @@ def _capture_envelope(envelope): | |||
| except Exception as e: | |||
| logger.debug("Can not set up continuous profiler. (%s)", e) | |||
|
|
|||
| from sentry_sdk.opentelemetry.integration import ( | |||
There was a problem hiding this comment.
I have two possible ideas:
- Import the
sentry_sdk.opentelemetry.integrationmodule at the top, then call the methods on the module - Move the import to the bottom of the file in a "circular imports" section (I believe we have used this pattern elsewhere before, so it is likely preferable to an inline import if possible to do)
There was a problem hiding this comment.
Agreed naming is hard – maybe we can call it setup or something like that, given that it sets up the link with Otel?
I just think integration is particularly confusing here. While it is true that we "integrate" with Opentelemetry, the terminology is confusing because in the SDK codebase, "integration" usually means an optional integration with a library like FastAPI, Celery, etc.
Moving stuff out of
integrations/opentelemetry/step by step since there is no OpenTelemetry integration anymore -- it's part of the core SDK.sentry_sdk/integrations/opentelemetry/integration.py->sentry_sdk/opentelemetry/tracing.py.integration.py.integrations/opentelemetry/altogether (there was nothing left but__init__.py, which is now also gone).tests/integrations/opentelemetrytotests/opentelemetry.tox.ini. These will now be run as part of the Common test suite.Ref #3853