Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KREST-10243 Add custom-request-logging to kafka-rest, and log error-codes for when various rate-limiters are triggered #1168

Merged
merged 11 commits into from Jun 20, 2023

Conversation

msn-tldr
Copy link
Member

@msn-tldr msn-tldr commented Jun 7, 2023

Following categories of changes are introduced in this PR.

  1. The new request-logger that will log custom metadata as part of the request-log.
  • Adds CustomLog.java, as a new Jetty request-logger, that also logs configured request-attributes.
    Example:
    Existing
    GET /kafka/v3/clusters/lkc-foobar/consumer-groups/foobar/lags HTTP/1.1" 429 408 "-" "Mozilla/5.0 (Windows NT; Windows NT 10.0; en-US) WindowsPowerShell/5.1.14393.5582
    Vs NOTE the new log has the error-code 429007 at the end, which is error-code defined in RateLimitExceededException.java class.
    GET /kafka/v3/clusters/lkc-foobar/consumer-groups/foobar/lags HTTP/1.1" 429 408 "-" "Mozilla/5.0 (Windows NT; Windows NT 10.0; en-US) WindowsPowerShell/5.1.14393.5582 429007
  1. Populate the relevant meta-data on request. This requires new request-attribute i.e. REST_ERROR_CODE to be populated when a rate-limit is being triggered. All rate-limiters, when triggered have a corresponding "error-code" in request-log, this logic is tested in RestCustomRequestLogIntegrationTest.java.
  • RateLimitExceededException.java has the error-codes that will be logged when rate-limit is triggered.
  • GlobalDosFilterListener.java & PerConnectionDosfilterListener.java are DosFilter.Listeners that on global & per-ip Jetty rate-limiters populate request-attribute needed for logging.
  • FixedCostRateLimitRequestFilter has changes to log error-code when Jersey request rate-limits are triggered.
  • Relevant changes to ProduceRatelimiters to log error-code when produce rate limits are triggered.
  • Tests
  1. This new custom-logger can be toggled on/off via use.custom.request.logging. By default it's ON.

@msn-tldr msn-tldr force-pushed the custom_logging branch 4 times, most recently from 1cedf14 to 71d5089 Compare June 10, 2023 16:35
kafka-rest/pom.xml Outdated Show resolved Hide resolved
@msn-tldr msn-tldr changed the title add custom request log KREST-10243 Add custom-request-logging to kafka-rest, and log error-codes for when various rate-limiters are triggered Jun 10, 2023
@msn-tldr msn-tldr marked this pull request as ready for review June 10, 2023 16:49
@msn-tldr msn-tldr requested a review from a team as a code owner June 10, 2023 16:49
kafka-rest/pom.xml Outdated Show resolved Hide resolved
@trnguyencflt
Copy link
Member

@msn-tldr could you sync this branch with master so that the build can pass? as rest-utils PR has been merged.

@msn-tldr
Copy link
Member Author

msn-tldr commented Jun 19, 2023

@trnguyencflt the build-failure is not do with rest-utils but very checkstyle issue -
ERROR] /home/jenkins/workspace/po_PR_builder_kafka-rest_PR-1168/kafka-rest/src/test/java/io/confluent/kafkarest/resources/v3/ProduceActionTest.java:40:8: Unused import - com.google.api.Http. [UnusedImports]

Weirdly, the import is not in my local repo, only here. When i try to update the remote repo(even forcefully), github thinks everything is upto date, and so the unused import doesn't go away.

Edit: Now fixed. Also had to rebase off master, since the version was bumped from 7.5.0 to 7.6.0, to pick latest build of rest-utils, and compile successfully.

Copy link
Member

@trnguyencflt trnguyencflt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@msn-tldr msn-tldr merged commit f5f6ce9 into confluentinc:master Jun 20, 2023
3 checks passed
@msn-tldr msn-tldr deleted the custom_logging branch June 20, 2023 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants