featzhang commented on PR #35:
URL:
https://github.com/apache/flink-connector-http/pull/35#issuecomment-4293179065
Thank you for the feedback and for pointing me to the existing logging
configuration. I understand the concern about introducing a parallel logging
system instead of extending what's already there.
After reviewing the existing `http.logging.level` configuration and
`HttpLogger`, I see that these serve a specific purpose:
- **Content verbosity control** (MIN/REQ_RESP/MAX) — how much detail to log
- **Always at DEBUG level** — intended for development-time troubleshooting
- **Logs all requests** (success and failure alike)
The challenge I was trying to address is **production error visibility**:
1. In production, DEBUG logging is typically disabled to avoid log volume
2. When DEBUG is off, HTTP errors become invisible (no ERROR/WARN level
output)
3. Log-based alerting systems (Splunk, Datadog, etc.) can't detect failures
because there's no ERROR-level signal
However, I completely agree that introducing `HttpErrorLogger` as a separate
class creates unnecessary duplication and divergence from the existing pattern.
Here's what I propose instead:
**Integrate error logging into the existing `HttpLogger` class**:
1. Keep `http.logging.level` (MIN/REQ_RESP/MAX) for content verbosity
2. Add a new config option: `http.error.log.severity` (OFF | ERROR | WARN |
INFO)
- Controls the SLF4J level for error logging
- Default: ERROR (log errors at ERROR level for production visibility)
- Set to OFF to disable (maintain current DEBUG-only behavior)
3. Extend `HttpLogger.logExceptionResponse()` to:
- Respect `http.error.log.severity` for the log level
- Use `http.logging.level` to determine content detail (body inclusion,
etc.)
- Keep continue-on-error awareness (only DEBUG when error is tolerated)
This way:
- **One logging system** (HttpLogger), not two
- **Two orthogonal dimensions**: content verbosity (MIN/REQ_RESP/MAX) +
error severity (OFF/ERROR/WARN/INFO)
- **Backward compatible**: Default behavior unchanged (errors still logged
at DEBUG)
- **Production-friendly**: Set `http.error.log.severity=ERROR` to surface
failures in monitoring
Does this approach sound better to you? I'm happy to refactor the PR along
these lines to keep everything within the existing `HttpLogger` infrastructure.
--
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]
