feat: Allow configuring `keep_alive` via environment variable #4366

szokeasaurusrex · 2025-05-06T09:22:32Z

This commit enables the keep_alive option to be set via the SENTRY_KEEP_ALIVE environment variable. When both the environment variable and the argument are provided, the argument takes precedence.

Closes #4354

This commit enables the `keep_alive` option to be set via the `SENTRY_KEEP_ALIVE` environment variable. When both the environment variable and the argument are provided, the argument takes precedence. Closes #4354

szokeasaurusrex · 2025-05-06T09:24:09Z

tests/test_client.py

+        # Argument overrides environment variable
+        ("0", True, True),  # env false, arg true
+        ("1", False, False),  # env true, arg false


Do you all think this order of precedence is reasonable? Or, should we not guarantee any specific order of precedence (i.e. if both are set to different values, behavior is undefined)?

In my opinion the env var should take precedence over the option set in the script (this way it allows the user to change the behavior by setting env vars, no matter was is set or not set in the script)

But if I look at our env var handling (SENTRY_ENVIRONMENT et al), if an option is set in the script, the env vars are not looked at at all.

So your order is OK. Consistency trumps other concerns.

I suppose the middle option would be to remove the tests asserting order and not provide any specific guarantee on precedence for now.

That way we could change the precedence later even in a minor release

What do you think @antonpirker?

But we have an order right now: Options set when calling init() are overruling env vars.

And I think we can not change this in a minor release, because it can alter behavior for users that have a init option and an env var set.

the tests are good, because if we change the precedence by accident, the tests will fail.

sounds good, then we stay with what we currently have

codecov · 2025-05-06T09:25:30Z

❌ 6 Tests Failed:

Tests completed	Failed	Passed	Skipped
1979	6	1973	15

View the top 3 failed test(s) by shortest run time

tests.integrations.redis.cluster.test_redis_cluster::test_rediscluster_pipeline[False-expected_first_ten0]

Stack Traces | 0.089s run time

.../redis/cluster/test_redis_cluster.py:121: in test_rediscluster_pipeline
    pipeline = rc.pipeline()
.../redis/cluster/test_redis_cluster.py:16: in <lambda>
    redis.RedisCluster.pipeline = lambda *_, **__: pipeline_cls(None, None)
.tox/py3.12-redis-latest/lib/python3.12.../site-packages/redis/utils.py:188: in wrapper
    return func(*args, **kwargs)
.tox/py3.12-redis-latest/lib/python3.12.../site-packages/redis/cluster.py:2125: in __init__
    retries=self.cluster_error_retry_attempts,
E   AttributeError: 'ClusterPipeline' object has no attribute 'cluster_error_retry_attempts'

tests.integrations.redis.cluster.test_redis_cluster::test_rediscluster_pipeline[True-expected_first_ten1]

Stack Traces | 0.089s run time

.../redis/cluster/test_redis_cluster.py:121: in test_rediscluster_pipeline
    pipeline = rc.pipeline()
.../redis/cluster/test_redis_cluster.py:16: in <lambda>
    redis.RedisCluster.pipeline = lambda *_, **__: pipeline_cls(None, None)
.tox/py3.12-redis-latest/lib/python3.12.../site-packages/redis/utils.py:188: in wrapper
    return func(*args, **kwargs)
.tox/py3.12-redis-latest/lib/python3.12.../site-packages/redis/cluster.py:2125: in __init__
    retries=self.cluster_error_retry_attempts,
E   AttributeError: 'ClusterPipeline' object has no attribute 'cluster_error_retry_attempts'

tests.integrations.redis.cluster.test_redis_cluster::test_rediscluster_span_origin

Stack Traces | 0.09s run time

.../redis/cluster/test_redis_cluster.py:162: in test_rediscluster_span_origin
    pipeline = rc.pipeline(transaction=False)
.../redis/cluster/test_redis_cluster.py:16: in <lambda>
    redis.RedisCluster.pipeline = lambda *_, **__: pipeline_cls(None, None)
.tox/py3.12-redis-latest/lib/python3.12.../site-packages/redis/utils.py:188: in wrapper
    return func(*args, **kwargs)
.tox/py3.12-redis-latest/lib/python3.12.../site-packages/redis/cluster.py:2125: in __init__
    retries=self.cluster_error_retry_attempts,
E   AttributeError: 'ClusterPipeline' object has no attribute 'cluster_error_retry_attempts'

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

antonpirker

Nice!

antonpirker · 2025-05-06T11:02:39Z

sentry_sdk/client.py

@@ -142,6 +142,11 @@ def _get_options(*args, **kwargs):
        )
        rv["socket_options"] = None

+    if rv["keep_alive"] is None:
+        rv["keep_alive"] = (
+            env_to_bool(os.environ.get("SENTRY_KEEP_ALIVE"), strict=True) or False


Suggested change

env_to_bool(os.environ.get("SENTRY_KEEP_ALIVE"), strict=True) or False

env_to_bool(os.environ.get("SENTRY_KEEP_ALIVE", "False"), strict=True)

to be consistent with SENTRY_DEBUG handling in the same file.

I'm not sure we should make this change because env_to_bool(os.environ.get("SENTRY_KEEP_ALIVE"), strict=True) or False and env_to_bool(os.environ.get("SENTRY_KEEP_ALIVE", "False"), strict=True) are not equivalent.

For example, if SENTRY_KEEP_ALIVE is set to an invalid value like "invalid", then the current code with or False evaluates to False, whereas your proposal would evaluate to None. Since keep_alive should have a boolean value, I think the current option is preferable.

If anything, I would consider changing the SENTRY_DEBUG handling to match what I have implemented here (although that would need a separate PR)

The important part is that all env vars are handled in the same way.
Its fine if you change the others, as long as all are behaving the same.

antonpirker · 2025-05-06T11:13:47Z

tests/test_client.py

+        # Argument overrides environment variable
+        ("0", True, True),  # env false, arg true
+        ("1", False, False),  # env true, arg false


In my opinion the env var should take precedence over the option set in the script (this way it allows the user to change the behavior by setting env vars, no matter was is set or not set in the script)

But if I look at our env var handling (SENTRY_ENVIRONMENT et al), if an option is set in the script, the env vars are not looked at at all.

So your order is OK. Consistency trumps other concerns.

antonpirker

see my comments, otherwise looks good!

feat: Allow configuring keep_alive via environment variable

f9cf4fb

This commit enables the `keep_alive` option to be set via the `SENTRY_KEEP_ALIVE` environment variable. When both the environment variable and the argument are provided, the argument takes precedence. Closes #4354

szokeasaurusrex requested a review from a team as a code owner May 6, 2025 09:22

szokeasaurusrex requested review from antonpirker and sentrivana and removed request for a team May 6, 2025 09:22

szokeasaurusrex commented May 6, 2025

View reviewed changes

antonpirker approved these changes May 6, 2025

View reviewed changes

szokeasaurusrex requested a review from antonpirker May 6, 2025 11:37

antonpirker approved these changes May 6, 2025

View reviewed changes

Merge branch 'master' into szokeasaurusrex/keep-alive-env-var

dacb5b1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow configuring `keep_alive` via environment variable #4366

feat: Allow configuring `keep_alive` via environment variable #4366

szokeasaurusrex commented May 6, 2025

szokeasaurusrex May 6, 2025

antonpirker May 6, 2025

szokeasaurusrex May 6, 2025 •

edited

Loading

antonpirker May 6, 2025

szokeasaurusrex May 6, 2025

codecov bot commented May 6, 2025 •

edited

Loading

antonpirker left a comment

antonpirker May 6, 2025

szokeasaurusrex May 6, 2025

antonpirker May 6, 2025

antonpirker May 6, 2025

antonpirker left a comment

	env_to_bool(os.environ.get("SENTRY_KEEP_ALIVE"), strict=True) or False
	env_to_bool(os.environ.get("SENTRY_KEEP_ALIVE", "False"), strict=True)

feat: Allow configuring keep_alive via environment variable #4366

Are you sure you want to change the base?

feat: Allow configuring keep_alive via environment variable #4366

Conversation

szokeasaurusrex commented May 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szokeasaurusrex May 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 6, 2025 • edited Loading

❌ 6 Tests Failed:

antonpirker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonpirker left a comment

Choose a reason for hiding this comment

feat: Allow configuring `keep_alive` via environment variable #4366

feat: Allow configuring `keep_alive` via environment variable #4366

szokeasaurusrex May 6, 2025 •

edited

Loading

codecov bot commented May 6, 2025 •

edited

Loading