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

[kafka] discovery of groups, topics and partitions #612

Merged
merged 10 commits into from
Jul 20, 2017
Merged

Conversation

truthbk
Copy link
Member

@truthbk truthbk commented Jul 20, 2017

What does this PR do?

This PR supersedes #271 - contains @jeffwidman work plus added testing infrastructure and submission of events in certain notable scenarios.

Main goal is to allow collecting all consumer groups, topics, partitions, without the need to include them in the configuration YAML. Note that this will hit zookeeper hard. Please see #271 for more details.

Motivation

@jeffwidman work

jeffwidman and others added 8 commits June 15, 2017 10:52
The consumer_groups, topics, and partitions arguments are each now
optional. Any omitted values will be fetched from Zookeeper. If a value
is omitted, the parent value must still be it's expected type (typically
a dict).

Modern Kafka consumers store their offset in Kafka rather than
Zookeeper, but this PR does not add support for this. It's tricky
because this check is much simpler if it assumes a single place can be
queried for all consumer offsets, but currently there's no easy way to
do that. Once KIP-88 is implemented, it will be much easier to add this
functionality, although it would only work for Kafka brokers >=
0.10.2.0. In the meantime, you can instrument your individual kafka
consumers to submit their offsets to Datadog.
@bits-bot
Copy link
Collaborator

@truthbk, thanks for your PR! By analyzing the history of the files in this pull request, we identified @gmmeyer to be a potential reviewer.

@truthbk
Copy link
Member Author

truthbk commented Jul 20, 2017

Logic to the substantial work by @jeffwidman was reviewed in #271, this adds testing logic, and infrastructure, but the code is essentially untouched. So I'll be merging this. Thank you Jeff!

@truthbk truthbk merged commit d82e567 into master Jul 20, 2017
@truthbk truthbk deleted the jaime/kafka271 branch July 20, 2017 14:39
@truthbk truthbk added this to the 5.16 milestone Jul 20, 2017
@jeffwidman
Copy link
Contributor

jeffwidman commented Jul 25, 2017

Thanks for pushing this across the finish line @truthbk, I really, really appreciate it. Sorry I didn't get a chance to get back to it until now.

I am a little sad that it was merged via squashing, as I had carefully kept each of the commits as atomic units so it was easier to follow the history. Versus now, with all your commits merged in there as a well it makes git blame a bit of a mess if you're trying to figure out why a particular piece of logic changed.

It also means I get no credit for the work in the git history. Since I'm spending time at my day job patching this, it's better if my changes are directly attributed to me in git blame / git log so I can internally help justify the time spent, versus now if someone gives it a cursory glance they'd be like "why did you waste time patching that, the datadog guys patched it themselves". Anyway, something for you and @jeremy-lq to think about in the future on how you manage community PRs...

Again, I am really happy to see this land, and I appreciate you handling the logic around sending an event, etc.

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.

3 participants