Skip to content

Commit

Permalink
feat: Banish fetching untyped runtime config (#4540)
Browse files Browse the repository at this point in the history
User should specify the type when fetching config. Now there are 3 new methods: get_int_config(), get_float_config() and get_str_config(). get_config() is deprecated as returning Any is a source of bugs and usually requires the user to cast to the correct type in their own code anyway.

We also want to call this method from the Rust consumer so we can access runtime config there, and we should not allow these untyped values there either.
  • Loading branch information
lynnagara authored Jul 24, 2023
1 parent 2484a57 commit ffcfdb9
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 8 deletions.
5 changes: 3 additions & 2 deletions snuba/consumers/consumer.py
Original file line number Diff line number Diff line change
Expand Up @@ -535,8 +535,9 @@ def process_message(
},
)

validate_sample_rate = float(
state.get_config(f"validate_schema_{snuba_logical_topic.name}", 1.0) or 0.0
validate_sample_rate = (
state.get_float_config(f"validate_schema_{snuba_logical_topic.name}", 1.0)
or 0.0
)

assert isinstance(message.value, BrokerValue)
Expand Down
7 changes: 4 additions & 3 deletions snuba/datasets/processors/spans_processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -403,10 +403,11 @@ def process_message(

except Exception as e:
metrics.increment("message_processing_error")
log_bad_span_pct = state.get_config(
"log_bad_span_message_percentage", default=0.0
log_bad_span_pct = (
state.get_float_config("log_bad_span_message_percentage", default=0.0)
or 0.0
)
if random.random() < float(log_bad_span_pct if log_bad_span_pct else 0.0):
if random.random() < log_bad_span_pct:
# key fields in extra_bag are prefixed with "spans_" to avoid conflicts with
# other fields in LogRecords
extra_bag = {"spans_" + str(k): v for k, v in message[2].items()}
Expand Down
6 changes: 3 additions & 3 deletions snuba/replacer.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
ReplacementMessage,
ReplacementMessageMetadata,
)
from snuba.state import get_config
from snuba.state import get_int_config, get_str_config
from snuba.utils.bucket_timer import Counter
from snuba.utils.metrics import MetricsBackend
from snuba.utils.rate_limiter import RateLimiter
Expand Down Expand Up @@ -414,7 +414,7 @@ def process_message(
"offset": metadata.offset,
},
)
if get_config("skip_seen_offsets", False):
if get_int_config("skip_seen_offsets"):
return None
seq_message = json.loads(message.payload.value)
[version, action_type, data] = seq_message
Expand Down Expand Up @@ -529,7 +529,7 @@ def _reset_offset_check(self, key: str) -> None:
temporarily, then cleared once relevant consumers restart.
"""
# expected format is "[consumer_group1,consumer_group2,..]"
consumer_groups = (get_config(RESET_CHECK_CONFIG) or "[]")[1:-1].split(",")
consumer_groups = (get_str_config(RESET_CHECK_CONFIG) or "[]")[1:-1].split(",")
if self.__consumer_group in consumer_groups:
self.__last_offset_processed_per_partition[key] = -1
redis_client.delete(key)
Expand Down
28 changes: 28 additions & 0 deletions snuba/state/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,36 @@ def set_configs(
set_config(k, v, user=user, force=force, config_key=config_key)


def get_int_config(
key: str, default: Optional[int] = None, config_key: str = config_hash
) -> Optional[int]:
config = _get_config(key, default, config_key)
return int(config) if config is not None else None


def get_float_config(
key: str, default: Optional[float] = None, config_key: str = config_hash
) -> Optional[float]:
config = _get_config(key, default, config_key)
return float(config) if config is not None else None


def get_str_config(
key: str, default: Optional[str] = None, config_key: str = config_hash
) -> Optional[str]:
config = _get_config(key, default, config_key)
return str(config) if config is not None else None


# To be deprecated, use get_int_config, get_float_config, get_str_config instead
def get_config(
key: str, default: Optional[Any] = None, config_key: str = config_hash
) -> Optional[Any]:
return _get_config(key, default, config_key)


def _get_config(
key: str, default: Optional[Any] = None, config_key: str = config_hash
) -> Optional[Any]:
return get_all_configs(config_key=config_key).get(key, default)

Expand Down
5 changes: 5 additions & 0 deletions tests/state/test_state.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,11 @@ def test_config_types(self) -> None:
# Tests for ints
state.set_config("test_int", 1)
assert state.get_config("test_int") == 1
assert state.get_int_config("test_int") == 1
state.set_config("test_int", 2)
state.set_config("test_int", "3")
assert state.get_config("test_int", 3)
assert state.get_int_config("test_int") == 3
with pytest.raises(MismatchedTypeException):
state.set_config("test_int", 0.1)
with pytest.raises(MismatchedTypeException):
Expand All @@ -65,9 +67,11 @@ def test_config_types(self) -> None:
# Tests for floats
state.set_config("test_float", 0.1)
assert state.get_config("test_float") == 0.1
assert state.get_float_config("test_float") == 0.1
state.set_config("test_float", 0.2)
state.set_config("test_float", "0.3")
assert state.get_config("test_float") == 0.3
assert state.get_float_config("test_float") == 0.3

with pytest.raises(MismatchedTypeException):
state.set_config("test_float", 1)
Expand All @@ -78,6 +82,7 @@ def test_config_types(self) -> None:
# Tests for strings
state.set_config("test_str", "some_string")
assert state.get_config("test_str") == "some_string"
assert state.get_str_config("test_str") == "some_string"
state.set_config("test_str", "some_other_string")
with pytest.raises(MismatchedTypeException):
state.set_config("test_str", 1)
Expand Down

0 comments on commit ffcfdb9

Please sign in to comment.