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.

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

init grafana service #108

Merged
merged 11 commits into from
Feb 22, 2024
Merged

Conversation

conscious-puppet
Copy link
Contributor

resolves #59

{
command = startScript;
readiness_probe = {
exec.command = "${pkgs.curl}/bin/curl -f ${grafanaConfig.server.protocol}://${grafanaConfig.server.domain}:${builtins.toString grafanaConfig.server.http_port}/api/health";
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Also, perhaps make the whole ${grafanaConfig.server.protocol}://${grafanaConfig.server.domain}:${builtins.toString grafanaConfig.server.http_port}" a value of an internal option (grafanaUri?), so it can be re-used in other places, including by the user. It also makes the probe CLI here more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srid sure.

grafana has a config called root_url for this. i'll store the resulting value in this config

http_port = config.port;
domain = "localhost";
};
} // config.extraConf;
Copy link
Member

Choose a reason for hiding this comment

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

What happens if extraConfg = { server.http_port = 8080; }? Would it not end up removing protocol and domain? Shouldn't we do a deep merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was not sure about this. i'll test this out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i am using lib.recursiveUpdate now.

i have made this same error here. i'll create a PR to fix this

Comment on lines 33 to 37
root_url = lib.mkOption {
type = types.str;
description = "The full public facing url.";
default = "${config.protocol}://${config.domain}:${builtins.toString config.http_port}";
};
Copy link
Member

Choose a reason for hiding this comment

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

If we don't expect the user to set this, you want to add readOnly = true; here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

grafana actually allows to give a root_url with subpath. in that case, user may set this value.

ref: https://grafana.com/docs/grafana/latest/setup-grafana/configure-grafana/#root_url

Copy link
Member

Choose a reason for hiding this comment

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

Will the user simply not set extraConfig.server.root_url instead? This additional option might not be needed. I think the latest impl. of readiness_probe looks good as is

doc/grafana.md Outdated
Comment on lines 24 to 62
1. Create `postgres` service.

```nix
services.postgres."pg1" = {
enable = true;
listen_addresses = "127.0.0.1";
port = 5435;
initialDatabases = [{
name = "grafana-db";
}];
initialScript.after = ''
CREATE USER gfuser with PASSWORD 'gfpassword' SUPERUSER;
'';
};
```

2. Create `grafana` service, and change its config to use the `postgres` database.

```nix
services.grafana."gf1" = {
enable = true;
http_port = 3001;
extraConf = {
database = {
type = "postgres";
host = "127.0.0.1:5435";
name = "grafana-db";
user = "gfuser";
password = "gfpassword";
};
};
};
```

3. Add a setting to start `grafana` only after `postgres` is running.

```nix
settings.processes."gf1".depends_on."pg1".condition = "process_healthy";
```
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. Create `postgres` service.
```nix
services.postgres."pg1" = {
enable = true;
listen_addresses = "127.0.0.1";
port = 5435;
initialDatabases = [{
name = "grafana-db";
}];
initialScript.after = ''
CREATE USER gfuser with PASSWORD 'gfpassword' SUPERUSER;
'';
};
```
2. Create `grafana` service, and change its config to use the `postgres` database.
```nix
services.grafana."gf1" = {
enable = true;
http_port = 3001;
extraConf = {
database = {
type = "postgres";
host = "127.0.0.1:5435";
name = "grafana-db";
user = "gfuser";
password = "gfpassword";
};
};
};
```
3. Add a setting to start `grafana` only after `postgres` is running.
```nix
settings.processes."gf1".depends_on."pg1".condition = "process_healthy";
```
```nix
{
services.postgres.pg1 = {
enable = true;
listen_addresses = “127.0.0.1”;
};
services.grafana.gf1 = {
enable = true;
extraConf.database = with config.services.postgres.pg1; {
type = “postgres”;
host = “${listen_addresses}:${port}”;
};
};
settings.processes."gf1".depends_on."pg1".condition = "process_healthy";
}
```

It would be nice to keep these tips/suggestions simple and short.
I have assumed that not specifying name and user for postgres db in grafana will use postgres by default for both, if not we can explicitly mention.

Copy link
Member

Choose a reason for hiding this comment

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

initialDatabases and intialScript can be part of postgres doc (whenever we add it) and if one wants to play around with postgres configs, they can refer from there.

I say this because, we are better off keeping these mini-tutorials short (with default configs) and not overwhelm the reader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, by default grafana looks for the user root in database. and for the database grafana. i have added config these two over your suggestion. please have a look

Copy link
Member

Choose a reason for hiding this comment

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

Usually we create folder for a service when there are more than just the service definition file and test file. We can preserve the flat hierarchy here.

doc/grafana.md Outdated
@@ -0,0 +1,40 @@
# Grafana Open Source
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why we can't just use Grafana here?

Copy link
Contributor Author

@conscious-puppet conscious-puppet Feb 22, 2024

Choose a reason for hiding this comment

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

i copied the doc from here:

i think, they probably want to distinguish this from Grafana Cloud. I'll change* it to grafana, sure

@shivaraj-bh shivaraj-bh merged commit 625f207 into juspay:main Feb 22, 2024
3 checks passed
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.

Add Grafana service
3 participants