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

[KOGITO-378] Integrate Kogito Operator with Prometheus Operator #98

Merged
merged 1 commit into from
Nov 5, 2019

Conversation

xieshenzh
Copy link
Contributor

Jira: https://issues.jboss.org/browse/KOGITO-378

Description: deploy a Service Monitor for scraping the metrics of the Kogitio app, if Prometheus Operator is installed.

Many thanks for submiting your Pull Request ❤️!

Please make sure that your PR meets the following requirements:

  • You have read the contributors guide
  • Pull Request title is properly formatted: [KOGITO-XYZ] Subject
  • Pull Request contains link to the JIRA issue
  • Pull Request contains description of the issue
  • Pull Request does not include fixes for issues other than the main ticket
  • Your feature/bug fix has a unit test that verifies it
  • You've tested the new feature/bug fix in an actual OpenShift cluster

Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

@xiezhang7 thanks for your work! Just a small question before I start the actual review. Please see below. Thanks!

Also, I believe that we have some failing tests.

pkg/client/client.go Show resolved Hide resolved
@sutaakar
Copy link
Contributor

Briefly went through the PR, didn't find there anything suspicious.
I will make more detailed review later, possible today or tomorrow.
If I won't make it this week then don't wait with merging for me.

@ricardozanini
Copy link
Member

@xiezhang7 is it possible to use the Discovery client to find if the Prometheus API is available in the cluster like we do today with isOpenShift method in the client package?

This way you just create the ServiceMonitor if the API is available using the CR and the standard client.

@xieshenzh
Copy link
Contributor Author

@xiezhang7 is it possible to use the Discovery client to find if the Prometheus API is available in the cluster like we do today with isOpenShift method in the client package?

This way you just create the ServiceMonitor if the API is available using the CR and the standard client.

@ricardozanini yes, we can do that. But we will still need the prometheus client to get the list of deployed service monitors, because the standard client doesn't support that.

@ricardozanini
Copy link
Member

@xiezhang7 is it possible to use the Discovery client to find if the Prometheus API is available in the cluster like we do today with isOpenShift method in the client package?
This way you just create the ServiceMonitor if the API is available using the CR and the standard client.

@ricardozanini yes, we can do that. But we will still need the prometheus client to get the list of deployed service monitors, because the standard client doesn't support that.

Cool! But why do we need to list the ServiceMonitors?

@xieshenzh
Copy link
Contributor Author

Cool! But why do we need to list the ServiceMonitors?

Because operator-utils is used to maintain the resources. It lists the resources and filters them by the ownership. It doesn't make any assumption on the resource name or the amount of the resources.

@ricardozanini
Copy link
Member

Cool! But why do we need to list the ServiceMonitors?

Because operator-utils is used to maintain the resources. It lists the resources and filters them by the ownership. It doesn't make any assumption on the resource name or the amount of the resources.

hmm I see. Well, I'm not that happy with this solution, but I see value using the operator-utils to manage our resources. It is what it is.

Could you please them rebase your commit?

@xieshenzh
Copy link
Contributor Author

Thanks, @ricardozanini . I have rebased the code, and changed the code to use Discovery client to check the Prometheus API.

Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

LGTM in general, @xiezhang7. Just minor comments.

Many thanks for the hard work!

pkg/controller/kogitoapp/resource/service_monitor_test.go Outdated Show resolved Hide resolved
pkg/resource/image_metadata.go Show resolved Hide resolved
.circleci/config.yml Show resolved Hide resolved
Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

Thanks @xiezhang7

@spolti
Copy link
Member

spolti commented Nov 4, 2019

I didn't find anything suspicious as well, but, as a user, it is no clear how it works, or what exactly needs to be done to enable/access the metrics, and how to take advantage of this integration, the user will need to do some manual configuration besides add the prometheus addon on their project, or will the operator take care of the prometheus operator integration and run it automatically?
Do you mind to add a few detailed information about it?

@xieshenzh
Copy link
Contributor Author

I didn't find anything suspicious as well, but, as a user, it is no clear how it works, or what exactly needs to be done to enable/access the metrics, and how to take advantage of this integration, the user will need to do some manual configuration besides add the prometheus addon on their project, or will the operator take care of the prometheus operator integration and run it automatically?
Do you mind to add a few detailed information about it?

Thanks, @spolti . I have updated the README.

  1. Accessing the metrics is configured by the prometheus operator. The integration only works when there is already a prometheus operator install in the cluster, which means the user is supposed to have some knowledge of the prometheus operator. Mainly, the user will have to create or configure a Prometheus instance for the prometheus operator. I added the information in the README .

  2. Enabling/generating metrics is configured in the kogito source project. It is mentioned in the first part of the Prometheus Integration section.

  3. The integration is to let the kogito operator create a Service Monitor instance of the prometheus operator automatically, when prometheus operator is installed and metrics for the kogito app is enabled. The Service Monitor instance is responsible for describing how the Prometheus instance should scrape the metrics of the Kogito App.

So the user is supposed to finish 1 and 2. The operator will do 3.

@ricardozanini ricardozanini removed the request for review from sutaakar November 5, 2019 15:17
@ricardozanini ricardozanini added ready 🚀 PR is ready to be merged and removed needs review 🔍 Pull Request that needs reviewers labels Nov 5, 2019
@ricardozanini ricardozanini removed the request for review from bmozaffa November 5, 2019 15:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ready 🚀 PR is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants