codeant-ai-for-open-source[bot] commented on code in PR #32487:
URL: https://github.com/apache/superset/pull/32487#discussion_r2657850323
##########
docker/entrypoints/run-server.sh:
##########
@@ -18,6 +18,9 @@
# under the License.
#
HYPHEN_SYMBOL='-'
+STATSD_HOST="${SERVER_STATSD_HOST:-localhost}"
+STATSD_PORT="${SERVER_STATSD_PORT:-8125}"
Review Comment:
**Suggestion:** Environment variables can contain leading/trailing
whitespace which will produce malformed host or port strings; trim whitespace
from `SERVER_STATSD_HOST` and `SERVER_STATSD_PORT` before composing the
address. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
STATSD_HOST="$(printf '%s' "${SERVER_STATSD_HOST:-localhost}" | sed -e
's/^[[:space:]]*//' -e 's/[[:space:]]*$//')"
STATSD_PORT="$(printf '%s' "${SERVER_STATSD_PORT:-8125}" | sed -e
's/^[[:space:]]*//' -e 's/[[:space:]]*$//')"
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Trimming whitespace from environment variables is a simple hardening step
that prevents accidental leading/trailing spaces from producing invalid
host/port strings. The sed-based trimming is portable and appropriate for this
entrypoint script.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docker/entrypoints/run-server.sh
**Line:** 21:22
**Comment:**
*Possible Bug: Environment variables can contain leading/trailing
whitespace which will produce malformed host or port strings; trim whitespace
from `SERVER_STATSD_HOST` and `SERVER_STATSD_PORT` before composing the address.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
docs/docs/configuration/event-logging.mdx:
##########
@@ -50,13 +50,28 @@ Superset can be configured to log events to
[StatsD](https://github.com/statsd/s
if desired. Most endpoints hit are logged as
well as key events like query start and end in SQL Lab.
-To setup StatsD logging, it’s a matter of configuring the logger in your
`superset_config.py`.
-If not already present, you need to ensure that the `statsd`-package is
installed in Superset's python environment.
+By default, Superset also collects gunicorn
[metrics](https://docs.gunicorn.org/en/stable/instrumentation.html).
+To enable these, the following environment variables should be set:
+
+```bash
+SERVER_STATSD_HOST=localhost
+SERVER_STATSD_PORT=8125
+SERVER_STATSD_PREFIX=superset
+```
+
+To setup StatsD logging for Superset, it’s a matter of configuring the logger
in your `superset_config.py`.
```python
+import os
from superset.stats_logger import StatsdStatsLogger
-STATS_LOGGER = StatsdStatsLogger(host='localhost', port=8125,
prefix='superset')
+STATS_LOGGER = StatsdStatsLogger(
+ host=os.environ.get("SERVER_STATSD_HOST", "localhost"),
+ port=int(os.environ.get("SERVER_STATSD_PORT", "8125")),
Review Comment:
**Suggestion:** Converting the environment variable to int with
int(os.environ.get(...)) will raise a ValueError if the environment variable is
set to an empty string or a non-numeric value, causing the configuration import
to fail; parse the variable defensively and fall back to a safe default
integer. [type error]
**Severity Level:** Minor ⚠️
```suggestion
_port_str = os.environ.get("SERVER_STATSD_PORT")
try:
_port = int(_port_str) if _port_str not in (None, "") else 8125
except ValueError:
_port = 8125
STATS_LOGGER = StatsdStatsLogger(
host=os.environ.get("SERVER_STATSD_HOST", "localhost"),
port=_port,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
int(os.environ.get(...)) will raise ValueError if the env var is set to ""
or a non-numeric value,
and that can break importing the config at runtime. The improved code
defensively parses the port
and falls back to a safe default, which prevents a real runtime failure in
production.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docs/docs/configuration/event-logging.mdx
**Line:** 67:69
**Comment:**
*Type Error: Converting the environment variable to int with
int(os.environ.get(...)) will raise a ValueError if the environment variable is
set to an empty string or a non-numeric value, causing the configuration import
to fail; parse the variable defensively and fall back to a safe default integer.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
