feat: Auto-enabling integrations behind feature flag#625
Conversation
|
👀
@untitaker does that mean the integrations that it enables may change over time?
Good question. What's the motivation for this new option? The PR description gave the what, we're missing the why. |
|
@rhcarvalho This was asked for in the context of APM where people would have to enable a lot of small integrations to get a meaningful span tree. Generally it's nice if people have to think about as little as possible (so, not about which integrations are necessary) when enabling the SDK. The semver compatibility is another thing. Yes, you could upgrade to a new version of the SDK and just get more integrations enabled automatically. Similar effects were already observable to some degree as we added more features to integrations that the user already explicitly enabled (breadcrumbs for django sql queries for example). I think we will just avoid this problem and make sure that new integrations or changes to existing ones don't break fundamental things like grouping, ever (like we already do) |
rhcarvalho
left a comment
rhcarvalho
left a comment
There was a problem hiding this comment.
A couple of questions and suggestions.
Got me thinking if Integration subclasses would know about their default "enabled" state maybe we could write less code.
| self.integrations = setup_integrations( | ||
| self.options["integrations"], | ||
| with_defaults=self.options["default_integrations"], | ||
| with_auto_enabling_integrations=self.options["_experiments"].get( |
There was a problem hiding this comment.
How is "_experiments" meant to be used? I see we use it for "max_spans" and "record_sql_params" already.
Do experimental options eventually migrate out of the "_experiments" namespace?
For maintenance's sake, could we document it?
There was a problem hiding this comment.
I added in-code documentation in consts.py but believe adding "real" documentation may give the impression anything in there is stable (despite any disclaimers).
For example we removed Relay from the main docs as it was alpha, even though each page of its documentation had a red "alpha" banner on top.
| assert event["exception"]["values"][0]["value"] == "aha! whatever" | ||
|
|
||
|
|
||
| def test_auto_enabling_integrations_does_not_break(sentry_init, caplog): |
There was a problem hiding this comment.
What's the intention of this test? "Does not break" is too open ended.
Seems like it checks that none of the integrations in the list get enabled.
There was a problem hiding this comment.
That they don't get enabled, but by break I rather meant that the DidNotEnable and ImportErrors are caught down properly. I'll rename
| "sentry_sdk.integrations.django.DjangoIntegration", | ||
| "sentry_sdk.integrations.flask.FlaskIntegration", | ||
| "sentry_sdk.integrations.bottle.BottleIntegration", | ||
| "sentry_sdk.integrations.falcon.FalconIntegration", | ||
| "sentry_sdk.integrations.sanic.SanicIntegration", | ||
| "sentry_sdk.integrations.celery.CeleryIntegration", | ||
| "sentry_sdk.integrations.rq.RqIntegration", | ||
| "sentry_sdk.integrations.aiohttp.AioHttpIntegration", | ||
| "sentry_sdk.integrations.tornado.TornadoIntegration", | ||
| "sentry_sdk.integrations.sqlalchemy.SqlalchemyIntegration", |
There was a problem hiding this comment.
Suggestion: extract this sequence such that the tests stay in sync with the implementation.
How to opt-out of a default integration today, say we don't want |
|
@rhcarvalho we don't have a solution for that either. We tell you to disable |
Fair enough. I wonder what would it look like if we simply "promoted" those now "auto-enabling" integrations to be default integrations. Meaning there would be only one concept: either an integration is part of the default set or not. Default integrations are enabled on a best-effort basis, such that integrations for modules you don't use would just not be enabled (e.g. the Flask integration, though being default on, would not be enabled in a typical Django project). To disable any integration, only a single answer: enumerate them explicitly, overriding the default set. But maybe the auto-enabling integrations have some other characteristic I didn't account for? |
This is the goal, and if it weren't for auto-integration being behind a feature flag it would already work |
Co-Authored-By: Rodolfo Carvalho <[email protected]>
Co-Authored-By: Rodolfo Carvalho <[email protected]>
Co-Authored-By: Rodolfo Carvalho <[email protected]>
Co-Authored-By: Rodolfo Carvalho <[email protected]>
Co-Authored-By: Rodolfo Carvalho <[email protected]>
Co-Authored-By: Rodolfo Carvalho <[email protected]>
|
@untitaker my bad, I overlooked that in the sanic integration there was already a https://travis-ci.com/getsentry/sentry-python/jobs/288969634 |
3 participants
The idea is that you can write
init(_experiments={"auto_enabling_integrations": True})to get maximal instrumentation.Unsolved questions: