Skip to content

request logs through kafka emitter#11036

Merged
pjain1 merged 8 commits intoapache:masterfrom
pjain1:request_logs_kafka_emitter
Apr 1, 2021
Merged

request logs through kafka emitter#11036
pjain1 merged 8 commits intoapache:masterfrom
pjain1:request_logs_kafka_emitter

Conversation

@pjain1
Copy link
Copy Markdown
Member

@pjain1 pjain1 commented Mar 26, 2021 •
edited
Loading

Fixes #10851

Description

Send request logs through Kafka Emitter, if request topic is set then the event will be emitted otherwise skipped and added to invalidLost count. When requestTopic is set, lost request logs are logged under requestLost metric.

Key changed/added classes in this PR
  • KafkaEmitter.java
  • DefaultRequestLogEvent.java

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

zhangyue19921010
zhangyue19921010 reviewed Mar 29, 2021
View reviewed changes
Comment thread extensions-contrib/kafka-emitter/src/main/java/org/apache/druid/emitter/kafka/KafkaEmitter.java Outdated
Comment thread server/src/main/java/org/apache/druid/server/log/DefaultRequestLogEvent.java Outdated
zhangyue19921010
zhangyue19921010 approved these changes Mar 29, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@zhangyue19921010 zhangyue19921010 left a comment

Choose a reason for hiding this comment

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

LGTM after CI passed :)

nishantmonu51
nishantmonu51 approved these changes Mar 29, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@nishantmonu51 nishantmonu51 left a comment

Choose a reason for hiding this comment

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

👍

@pjain1
Copy link
Copy Markdown
Member Author

pjain1 commented Mar 30, 2021

Tests failing due to insufficient test coverage.

@pjain1
Copy link
Copy Markdown
Member Author

pjain1 commented Mar 30, 2021

added unit test for kafka emitter

@pjain1
Copy link
Copy Markdown
Member Author

pjain1 commented Mar 31, 2021

All green, will merge by eod.

@capistrant
Copy link
Copy Markdown
Contributor

capistrant commented Mar 31, 2021

@pjain1 This is a great add, will be very useful! Can you update the description with the information on logging now that lost events are logged under requestLost after latest commits? That way we won't worry about someone reading this description and not looking at the code if they are troubleshooting.

@pjain1
Copy link
Copy Markdown
Member Author

pjain1 commented Mar 31, 2021

@capistrant updated, hope it makes sense.

Although looking at the emitter code again, I found an issue - memory bound for each queue is set to the Kafka producer buffer memory config. I believe the intention is keep the buffered events size less than or equal to the producer buffer but it will actually be 3 times the producer buffer. This problem would have been there since the implementation of this emitter so I think can be addressed in separate PR.

@capistrant
Copy link
Copy Markdown
Contributor

capistrant commented Mar 31, 2021

@capistrant updated, hope it makes sense.

Although looking at the emitter code again, I found an issue - memory bound for each queue is set to the Kafka producer buffer memory config. I believe the intention is keep the buffered events size less than or equal to the producer buffer but it will actually be 3 times the producer buffer. This problem would have been there since the implementation of this emitter so I think can be addressed in separate PR.

thanks! Do you think you can open up an issue with a quick write up on that suspected bug? I agree that it can be fixed in a separate PR.

I did ask a small question in a review comment. I don't mean to come in and nit at this since it is already approved. Just curious to know the reasoning on a small choice.

capistrant
capistrant approved these changes Mar 31, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@capistrant capistrant left a comment

Choose a reason for hiding this comment

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

LGTM after CI

@pjain1 pjain1 merged commit b35486f into apache:master Apr 1, 2021
@pjain1 pjain1 deleted the request_logs_kafka_emitter branch April 1, 2021 06:01
@clintropolis clintropolis added this to the 0.22.0 milestone Aug 12, 2021
jon-wei added a commit to jon-wei/druid that referenced this pull request Nov 22, 2021
@github-actions @viatcheslavmogilevsky
@pjain1 @techdocsmith @FrankChen021 @tushar-1728 @jon-wei @jihoonson @lkm @clintropolis @maytasm
Update from master-for-merge-to-release (apache#318)
9603203 
* IMPLY-6556 remove offending settings.xml for intellij inspections

* GCS lookup support (apache#11026)

* GCS lookup support

* checkstyle fix

* review comments

* review comments

* remove unused import

* remove experimental from Kinesis with caveats (apache#10998)

* remove experimental from Kinesis with caveats

* add suggested known issue

* spelling fixes

* Bump aliyun SDK to 3.11.3 (apache#11044)

* Update reset-cluster.md (apache#10990)

fixed Error: Could not find or load main class org.apache.druid.cli.Main

* Make imply-view-manager non-experimental (apache#316)

* Make druid.indexer.task.ignoreTimestampSpecForDruidInputSource default to true, for backwards compat (apache#315)

* Add explicit EOF and use assert instead of exception (apache#11041)

* Add Calcite Avatica protobuf handler (apache#10543)

* bump to latest of same version node and npm versions, bump frontend-maven-plugin (apache#11057)

* request logs through kafka emitter (apache#11036)

* request logs through kafka emitter

* travis fixes

* review comments

* kafka emitter unit test

* new line

* travis checks

* checkstyle fix

* count request lost when request topic is null

* IMPLY-6556 map local repository instead .m2

* remove outdated info from faq (apache#11053)

* remove outdated info from faq

* Add an option for ingestion task to drop (mark unused) all existing segments that are contained by interval in the ingestionSpec (apache#11025)

* Auto-Compaction can run indefinitely when segmentGranularity is changed from coarser to finer.

* Add option to drop segments after ingestion

* fix checkstyle

* add tests

* add tests

* add tests

* fix test

* add tests

* fix checkstyle

* fix checkstyle

* add docs

* fix docs

* address comments

* address comments

* fix spelling

* Allow list for JDBC connection properties to address CVE-2021-26919 (apache#11047)

* Allow list for JDBC connection properties to address CVE-2021-26919

* fix tests for java 11

* Fix compile issue from dropExisting in ingest-service (apache#320)

Co-authored-by: Slava Mogilevsky <triggerwoods91@gmail.com>
Co-authored-by: Parag Jain <pjain1@apache.org>
Co-authored-by: Charles Smith <38529548+techdocsmith@users.noreply.github.com>
Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: frank chen <frank.chen021@outlook.com>
Co-authored-by: Tushar Raj <43772524+tushar-1728@users.noreply.github.com>
Co-authored-by: Jonathan Wei <jon-wei@users.noreply.github.com>
Co-authored-by: Jihoon Son <jihoonson@apache.org>
Co-authored-by: Lasse Krogh Mammen <lkm@bookboon.com>
Co-authored-by: Clint Wylie <cwylie@apache.org>
Co-authored-by: Maytas Monsereenusorn <maytasm@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nishantmonu51 nishantmonu51 nishantmonu51 approved these changes

@capistrant capistrant capistrant approved these changes

@zhangyue19921010 zhangyue19921010 zhangyue19921010 approved these changes

Assignees

No one assigned

Labels

Area - Extension Area - Metrics/Event Emitting

Projects

None yet

Milestone

0.22.0

Development

Successfully merging this pull request may close these issues.

Add RequestLogEvent to Kafka Emitter to Emitter the Request

5 participants

@pjain1 @capistrant @nishantmonu51 @zhangyue19921010 @clintropolis