Skip to content
This repository has been archived by the owner on Mar 17, 2024. It is now read-only.

Allow whitelisting Kafka topics #65

Merged
merged 3 commits into from
Sep 18, 2019
Merged

Conversation

NeQuissimus
Copy link
Contributor

@NeQuissimus NeQuissimus commented Sep 17, 2019

I am running a snapshot Docker image of this right now, everything seems to work fine using the following application.conf

kafka-lag-exporter {
  clusters = [
    {
      name = "dev"
      bootstrap-brokers = "broker1:9092,broker2:9092,broker3:9092"
      consumer-properties = {
        client.id = "kafka-lag"
      }
      admin-client-properties = {
        client.id = "kafka-lag-admin"
      }
    }
  ]
  lookup-table-size = 200
  client-group-id = "test-kafkalagexporter"
  metric-whitelist = ["kafka_consumergroup_group_lag_seconds"]
  topic-whitelist = ["prefix1-.*", "prefix2-.*"]
}

Copy link
Owner

@seglo seglo left a comment

Choose a reason for hiding this comment

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

Hi Tim. Thanks for the PR. This along with #4 and #43 will provide a nice set of options for users to limit the number of metrics reported.

I have a few requests:

  1. Please add a test to KafkaClientSpec.
  2. I assume you probably don't use Helm, but would you mind adding the configuration to the Helm chart too in ./charts/kafka-lag-exporter/. It should be easy to follow the same convention as metricWhitelist in the values.yaml and templates/030-ConfigMap.yaml. Don't worry about testing it, I can do that.
  3. Add the configuration to the Helm configuration section in the README.md.

@NeQuissimus
Copy link
Contributor Author

Please check the changes to the Helm chart.
I added a few tests for different edge cases.

@seglo seglo self-requested a review September 18, 2019 16:22
Copy link
Owner

@seglo seglo left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot for the extra test coverage!

@seglo seglo merged commit 1430ea9 into seglo:master Sep 18, 2019
@NeQuissimus NeQuissimus deleted the topic-whitelist branch September 18, 2019 18:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants