Thrift metrics: omit status codes for exceptions with a non-int code (#…

…892) Co-authored-by: Chris Dary <[email protected]>
reddit · Apr 23, 2024 · a5abb86 · a5abb86
1 parent 377488c
commit a5abb86
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 3 deletions.
diff --git a/baseplate/clients/thrift.py b/baseplate/clients/thrift.py
@@ -300,7 +300,7 @@ def _call_thrift_method(self: Any, *args: Any, **kwargs: Any) -> Any:
  if exc_info[0] is not None:
  thrift_success = "false"
  exception_type = exc_info[0].__name__
- current_exc = exc_info[1]
+ current_exc: Any = exc_info[1]
  try:
  # We want the following code to execute whenever the
  # service raises an instance of Baseplate's `Error` class.
@@ -317,8 +317,17 @@ def _call_thrift_method(self: Any, *args: Any, **kwargs: Any) -> Any:
  # we will emit the status code in both cases
  # but the status will be blank in the first case, and the baseplate name
  # in the second
- baseplate_status_code = current_exc.code # type: ignore
- baseplate_status = ErrorCode()._VALUES_TO_NAMES.get(current_exc.code, "") # type: ignore
+ #
+ # Since this exception could be of any type, we may receive exceptions
+ # that have a `code` property that is actually not from Baseplate's
+ # `Error` class. In order to reduce (but not eliminate) the possibility
+ # of metric explosion, we validate it against the expected type for a
+ # proper Error code.
+ if isinstance(current_exc.code, int):
+ baseplate_status_code = str(current_exc.code)
+ baseplate_status = ErrorCode()._VALUES_TO_NAMES.get(
+ current_exc.code, ""
+ )
  except AttributeError:
  pass
 

diff --git a/tests/unit/clients/thrift_tests.py b/tests/unit/clients/thrift_tests.py
@@ -66,6 +66,11 @@ class ExampleClient:
  list(thrift._enumerate_service_methods(ExampleClient))
 
 
+class NonBaseplateExceptionWithCode(Exception):
+ def __init__(self):
+ self.code = lambda: "fake method"
+
+
 class TestPrometheusMetrics:
  def setup(self):
  REQUEST_LATENCY.clear()
@@ -127,6 +132,14 @@ def setup(self):
  "",
  pytest.raises(Exception),
  ),
+ # Ensure that we don't poison prom metrics with non-baseplate status codes
+ (
+ NonBaseplateExceptionWithCode(),
+ "NonBaseplateExceptionWithCode",
+ "",
+ "",
+ pytest.raises(NonBaseplateExceptionWithCode),
+ ),
  ],
  )
  def test_build_thrift_proxy_method(self, exc, exc_type, status, status_code, expectation):