Skip to content

CP-20 [WIP]: collecting metrics on server startup#79

Closed
Eno Thereska (enothereska) wants to merge 7 commits into
trunkfrom
PhoneHome
Closed

CP-20 [WIP]: collecting metrics on server startup#79
Eno Thereska (enothereska) wants to merge 7 commits into
trunkfrom
PhoneHome

Conversation

@enothereska

Copy link
Copy Markdown

No description provided.

Eno Thereska added 2 commits October 22, 2015 11:43
…ppers start a daemon thread that continuosly monitors vital broker properties. The output is specified in the log4j configuration file. It can be a file or a Kafka topic.
Comment thread bin/broker-server-start.sh Outdated

Choose a reason for hiding this comment

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

Any value in trying to reuse/refactor the existing script to keep this in sync with kafka-server-start without much additional effort? Not sure if it would make things better or worse than it would be with this approach since it might just mean painful merges each time anything changes upstream.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Looked into it, not worth refactoring the existing script IMO.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm a bit concerned about having two scripts for starting Kafka in a customer environment:

  • The one that appears in the quickstart documentation (kafka-server-start)
  • The one we want them to use (broker-server-start)

How about simply replacing the original script and having one script, with one well-documented name? It will still start Kafka, so users shouldn't mind.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Gwen Shapira (@gwenshap) Thanks, makes sense. We can have a patch for the original script but keep the script name the same.

…ame of metrics file and topic. Added Kafka version metric to collection
Comment thread config/log4j.properties Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The idea is that we'll tell customers "When you get support, uncomment these two lines"?

@gwenshap

Copy link
Copy Markdown

I have an additional concern:

Neha mentioned that the collection agent will be a separate JAR, but this patch implements a wrapper on Kafka and will be included in the same jar as the rest of Kafka broker.

I don't have a strong preference either way, but perhaps you want to sync on the approach?

One way to ship a separate JAR without having to run an extra process would be to implement a metrics reporter that takes the relevant metrics and produces (or logs ) them as you are doing now. Metric reporters are pluggable, so we can just add the name of the reporter class to the server.properties we ship and Kafka will load it and execute.

@enothereska

Copy link
Copy Markdown
Author

Gwen Shapira (@gwenshap) I had to open the patch against kafka to facilitate review since I do not have another place to put the code for now. This will be a separate jar (see pom file in a separate email thread). This PR should not be merged into kafka and is only for reviewing the code.

I am ok with the option of a metrics reporter, but it can only collect data reported through metrics registry as it won't have access to the broker object. I'm in the process of evaluating whether this access buys us much more or not. If not, I agree that going via the metrics reporter route is best.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you really need to add MODULE$ here? Scala tries to add static forwarder methods so that you can call such methods from Java as normal (ie Kafka.getPropsFromArgs(args)). There are cases where it can't do it due to name clashes, but it's worth trying (and make sure you compile the code, IDEA may misreport an error).

@enothereska

Copy link
Copy Markdown
Author

Comments received, thank you. Moved development to confluentinc/support repo.

@enothereska Eno Thereska (enothereska) deleted the PhoneHome branch October 27, 2015 10:29
José Armando García Sancio (jsancio) pushed a commit that referenced this pull request Sep 13, 2019
This was originally committed by Xavier to private-common, reviewed by Andy Coates:
confluentinc/private-common@3e55e05

Reviewers: Stanislav Kozlovski <stanislav_kozlovski@outlook.com>, Ismael Juma <ismael@juma.me.uk>, Xavier Léauté <xl+github@xvrl.net>
Andrew Egelhofer (andrewegel) pushed a commit that referenced this pull request Nov 16, 2021
List of commits from kafka-broker-plugins:

* Include all transitive dependencies in assembly (#71)
* Add OAuth LoginCallbackHandler for the Kafka server (#72)
* CPKAFKA-1593: Multi-tenant authorizer for CCloud (#66)
* CPKAFKA-1593: Integration tests for multi-tenant authorizer for CCloud (#67)
* CAAS-1960: Implement Phase 3.1 Metrics (#76)
* CAAS-1960: Add authenticated connections and authentication rate metrics per tenant (#77)
* Bump Confluent to 5.2.0-SNAPSHOT, Kafka to 2.2.0-SNAPSHOT
* Topic config constraints (#70)
* Remove redundant import in MultiTenantRequestContext (#79)
* 4.1.x to 5.0.x (#78)
* 5.0.x to 5.1.x (#82)
* CPKAFKA-1408: Add tenant throughput and request quota plugin (#68)
* Improve topic policy checks (#81)
* CPKAFKA-1410: Partition assignor to balance partitions at tenant level (#73)
* CPKAFKA-1694: Add partition throughput percentiles for monitoring (#84)
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.

4 participants