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.

Sign up for GitHub

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

Jump to bottom

Add kafka exporter #1947

Merged
merged 11 commits into from Sep 11, 2019
Merged

Add kafka exporter #1947

merged 11 commits into from Sep 11, 2019

Conversation

sknot-rh
Copy link
Member

Signed-off-by: Stanislav Knot sknot@redhat.com

Type of change

  • Enhancement / new feature

Description

Added Kafka exporter

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Update/write design documentation in ./design
  • Write tests
  • Make sure all tests pass
  • Update documentation
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md

@scholzj scholzj added this to the 0.14.0 milestone Aug 30, 2019
scholzj
scholzj reviewed Aug 30, 2019
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

I did a first quick pass and left some comments.

api/src/main/java/io/strimzi/api/kafka/model/KafkaExporterSpec.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/KafkaExporterSpec.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/KafkaExporterSpec.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/KafkaExporterSpec.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/KafkaExporterSpec.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/KafkaSpec.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/template/KafkaExporterTemplate.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/template/KafkaExporterTemplate.java Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/KafkaExporterResources.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/KafkaExporterResources.java Outdated Show resolved Hide resolved
@scholzj
Copy link
Member

scholzj commented Aug 30, 2019

@ppatierno @tombentley Stanislav named it KafkaExporter. I think that shows respect to the project and its author. But at the same time I'm wondering if calling it lagMonitoring would be more versatile if we ever need to change it in the future.Although maybe if we ever need to change it, we will anyway be unable keep the API compatible and it might be anyway needed to add a new onject for it. WDYT?

@tombentley
Copy link
Member

I think we should only call it lagMonitoring if we could (in principle) replace it with something else. To do that we'd need whatever else to support all the exposed metrics and with the same names. So I suspect that means we should call it kafkaExporter, though that's not an idea name because it doesn't suggest "lag metrics" to anyone unfamiliar with that project.

ppatierno
ppatierno reviewed Sep 2, 2019
View reviewed changes
api/src/main/java/io/strimzi/api/kafka/model/KafkaExporterResources.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/KafkaExporterSpec.java Show resolved Hide resolved
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaExporter.java Outdated Show resolved Hide resolved
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaExporter.java Outdated

String image = spec.getImage();
if (image == null) {
image = System.getenv().get("STRIMZI_DEFAULT_KAFKA_EXPORTER_IMAGE");
Copy link
Member

Choose a reason for hiding this comment

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

there is no constant defined elsewhere for this?

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaExporter.java Outdated Show resolved Hide resolved
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaExporter.java Outdated Show resolved Hide resolved
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaExporter.java Outdated Show resolved Hide resolved
PaulRMellor
PaulRMellor approved these changes Sep 5, 2019
View reviewed changes
Copy link
Contributor

@PaulRMellor PaulRMellor left a comment

Choose a reason for hiding this comment

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

👍

scholzj
scholzj reviewed Sep 7, 2019
View reviewed changes
api/src/main/java/io/strimzi/api/kafka/model/template/KafkaExporterTemplate.java Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/KafkaExporterSpec.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/KafkaExporterSpec.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/KafkaExporterSpec.java Outdated Show resolved Hide resolved
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaExporter.java Outdated Show resolved Hide resolved
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaExporter.java Outdated Show resolved Hide resolved
...rator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaAssemblyOperator.java Show resolved Hide resolved
@ppatierno ppatierno mentioned this pull request Sep 9, 2019
Closed
sknot-rh and others added 6 commits September 11, 2019 04:07
@sknot-rh @scholzj
Add kafka exporter
e953924 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh @scholzj
cleanup
38fb915 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh @scholzj
comments
60563e3 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh @scholzj
comments
7b48f41 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@scholzj
Deal with review comments, move to port 9091 and TLS authentication, …
992f0d4 
…handle situations when KE is not enabled and fix the startup script

Signed-off-by: Jakub Scholz <www@scholzj.com>
@scholzj
Fix API docs after rebase
1df3e1c 
Signed-off-by: Jakub Scholz <www@scholzj.com>
@scholzj
Copy link
Member

scholzj commented Sep 11, 2019

@ppatierno I (hopefully) addressed the comments, and fixed all the missing features:

  • SSL auth
  • Connecting to 9091 port
  • Startup srcipt
  • Handling of disabled KE
  • Container templates
  • etc.

Could you please review again?

@scholzj
Fix default image issues and KafkaAssemblyOperatorTest
b2b5a1d 
Signed-off-by: Jakub Scholz <www@scholzj.com>
@scholzj
Fix spotbugs
dfaf44e 
Signed-off-by: Jakub Scholz <www@scholzj.com>
@scholzj
Remove unused variable
6dbd1df 
Signed-off-by: Jakub Scholz <www@scholzj.com>
@scholzj
Copy link
Member

scholzj commented Sep 11, 2019

Ok, the tests and spotbugs now passing 🍾

@scholzj
Align the handling of env vars from template with the latest stuff us…
8cceaf6 
…ed i ther continers

Signed-off-by: Jakub Scholz <www@scholzj.com>
ppatierno
ppatierno approved these changes Sep 11, 2019
View reviewed changes
Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

I could of nits but overall LGTM.

api/src/main/java/io/strimzi/api/kafka/model/KafkaExporterResources.java Outdated Show resolved Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/KafkaExporterResources.java Outdated Show resolved Hide resolved
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaExporter.java Show resolved Hide resolved
@scholzj
Review comments
a8999c9 
Signed-off-by: Jakub Scholz <www@scholzj.com>
@scholzj scholzj merged commit 8eb55e4 into strimzi:master Sep 11, 2019
@ppatierno ppatierno mentioned this pull request Sep 12, 2019
Merged
@sknot-rh sknot-rh deleted the kafka-exporter-integration branch October 18, 2019 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scholzj scholzj scholzj left review comments

@PaulRMellor PaulRMellor PaulRMellor approved these changes

@ppatierno ppatierno ppatierno approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.14.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sknot-rh @scholzj @tombentley @ppatierno @PaulRMellor