Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Adding Prometheus and Grafana #1837

Merged
merged 16 commits into from
Nov 29, 2017
Merged

Adding Prometheus and Grafana #1837

merged 16 commits into from
Nov 29, 2017

Conversation

ritazh
Copy link
Member

@ritazh ritazh commented Nov 28, 2017

What this PR does / why we need it:
Adding Prometheus and Grafana to acs-engine to enable out of the box k8s monitoring experience

Special notes for your reviewer:
We have decided to create a new extension for this purpose to fully utilize the existing Prometheus and Grafana helm charts to manage releases and updates of existing charts. The new prometheus-grafana-k8s extension installs the helm cli and installs the Prometheus and Grafana charts.

cc @tstringer

@ritazh ritazh requested a review from jackfrancis November 28, 2017 16:03
@ghost ghost assigned ritazh Nov 28, 2017
@jackfrancis
Copy link
Member

Thanks @ritazh! Could you describe the UX for opting into having this extension installed on your acs-engine deployed cluster?

# prometheus-grafana Extension


This is the prometheus-grafana extension. Add this extension to your json file as shown below to automatically enable prometheus and grafana in your new Kubernetes cluster.
Copy link
Member

Choose a reason for hiding this comment

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

Could you kindly replace "your json file" with "the api model you pass as input into acs-engine"

@jackfrancis
Copy link
Member

@ritazh Ignore that, I see the api model pattern documented in the README!

@ritazh
Copy link
Member Author

ritazh commented Nov 28, 2017

@jackfrancis To opt out of this feature, you simple do not include extensions and extensionProfiles in the json.

```
$ kubectl get pods --show-all

$ NAMESPACE=default

Choose a reason for hiding this comment

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

Is this configurable? If not it probably should be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently it is not configurable. Definitely agreed that the end user should be able to specify a namespace.

@jackfrancis
Copy link
Member

@tstringer Would you add an example .json api model to the examples/extensions/ directory so that we can easily run E2E tests using this cluster configuration? Here's something to use as a reference, an example api model that uses the OMS extension:

https://github.com/Azure/acs-engine/blob/master/examples/extensions/kubernetes.oms.json

{
"name": "prometheus-grafana-k8s",
"version": "v1",
"rootURL": "https://raw.githubusercontent.com/ritazh/acs-engine/feat-monitor/"
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of the rootURL property? We probably want it to refer to something a little more persistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's where it'll pull the extension itself from. And absolutely that should be pointing to the upstream repo, but we were using @ritazh's fork to test this out.

This is a chicken and the egg thing. What is usually done to test out an extension in a fork before being merged upstream?

Copy link
Member

Choose a reason for hiding this comment

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

Understood. Because this is an optional property, can we remove it from the examples/ api model, or is there value in configuring your cluster to include this extension with the rootURL property properly defined?

Copy link
Member Author

Choose a reason for hiding this comment

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

rootURL is used by dta = strings.Replace(dta, "EXTENSION_URL_REPLACE", extensionProfile.RootURL, -1) in engine.go and EXTENSION_URL_REPLACE is used in template-link.json as:

"templateLink": {
            "uri": "EXTENSION_URL_REPLACEextensions/prometheus-grafana-k8s/v1/template.json",
            "contentVersion": "1.0.0.0"
        },

@trstringer
Copy link
Contributor

@jackfrancis absolutely, we can add a sample api model config to the repo. That'll be done shortly.

@trstringer
Copy link
Contributor

@jchauncey this pr now includes support to specify a non-default namespace to put these components in. Thanks for pointing that out!

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

lgtm

@ccojocar
Copy link

@jackfrancis Please could you also review this PR which adds Prometheus instrumentation to Azure API calls kubernetes/kubernetes#56286?

@jchauncey
Copy link

@cosmincojocar do metrics like this - mc := newMetricContext("public_ip_addresses", "list", az.ResourceGroup, az.SubscriptionID) capture the time it takes for the public ip address to be provisioned?

@ccojocar
Copy link

The metric will record the duration of the Azure API call (in this case list public IP addresses) and also if the call was successful or an error. The metrics are grouped per subscription and resource group.

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.

6 participants