Skip to content

Conversation

@malcolmholmes
Copy link
Contributor

Previously, a change was made to make it possible to have multiple Prometheus servers running. This involved two changes: putting everything into a prometheus wrapper element, and then making that element hidden.

The issue is that, for a new user, this introduces a weird behaviour. You get Grafana installed, but no Prometheus. And the reason, for a new Jsonnet user is esoteric: :: vs :.

This PR keeps all Prometheus resources inside the new element - this seems like a good practice, as it allows that element to be referenced and cloned, as was the intent of the original change.

However, it makes the prometheus element public. That way, newcomers will get the Prometheus server they expect. More experienced users, who want to set up multiple Prometheus servers, can do prometheus:: $.prometheus, which will hide the prometheus node.

@woodsaj
Copy link
Contributor

woodsaj commented Jan 15, 2020

We dont need to revert this change here. With our internal usage of prometheus-ksonnet we have the following:

  // We don't want to deploy main_prometheus, but we can't hide it in
  // prometheus-ksonnet or we'd break other peoples deployments.
  main_prometheus: {},

So, looks like the issue here is simply that main_prometheus is not defined. So perhaps we just add it.
eg in prometheus.libsonnet just add the following at the bottom.

  main_prometheus: $.prometheus {name: "prometheus"},

@cstyan
Copy link
Contributor

cstyan commented Jan 15, 2020

Came here to say the same thing as @woodsaj. I don't think this is the correct solution unless there were a way to enforce that name is unique for each Prometheus.

In this PR (#156) I claim to have made deployment of Prometheus replicas as a statefulset the default, but clearly I didn't add the actual replicas.

Re adding main_prometheus should work.

main_prometheus: $.prometheus {
    name: 'prometheus',
},

Copy link
Contributor

@cstyan cstyan left a comment

Choose a reason for hiding this comment

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

re-add main_prometheus and double check that that doesn't result in the addition of a third Prometheus instance in one of our clusters

@malcolmholmes
Copy link
Contributor Author

@cstyan Thanks. I feel better now - the main_prometheus thing makes sense of the history.

I'd say that my fix and the main_prometheus one achieve basically the same thing, but I'm happy to go with the original intent. Will fix tomorrow.

@malcolmholmes
Copy link
Contributor Author

@cstyan switched to your main_prometheus fix. Approved?

Comment on lines +2 to +11
/*
* All Prometheus resources are contained within a `prometheus` node. This allows
multiple Prometheus instances to be created by simply cloning this node, like
so:
`other_prometheus: $.prometheus {name: "other-prometheus"},`
To remove the default Prometheus, use this code:
`main_prometheus: {},`
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome 👍

@malcolmholmes malcolmholmes merged commit 76e8244 into master Jan 17, 2020
@malcolmholmes malcolmholmes deleted the make-prometheus-deploy-easily branch January 17, 2020 17:46
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.

3 participants