cockroachdb / terraform-provider-cockroach

Terraform provider for CockroachDB Cloud
Apache License 2.0
55 stars 10 forks source link

[CC-26984] metric-export: add resource for prometheus integration #200

Closed aa-joshi closed 1 month ago

aa-joshi commented 2 months ago

This change introduces resource MetricExportPrometheusConfig which manages prometheus metric export integration in gcp and aws cloud providers.

Commit checklist

fantapop commented 2 months ago

@aa-joshi @marksoper considering that there is no actual configuration for enabling prometheus what you do think of having it as an additional field on the cluster resource? It could be something like:

resource "cockroach_cluster" "dedicated" {
  name           = "cockroach-dedicated"
  cloud_provider = "GCP"
  prometheus_metric_export_enabled = true
  dedicated = {
  ....
  }

If we were planning to add configuration options to this at some point in the near future, it seems like keeping it as its own resource would be desirable but even in that case we could just have that resource just represent the configuration and keep the enabled / disabled flag on cluster.

For example,

resource "cockroach_cluster" "dedicated" {
  name           = "cockroach-dedicated"
  cloud_provider = "GCP"
  prometheus_metric_export_enabled = true
  dedicated = {
  ....
  }
resource "cockroach_cluster_prometheus_export_config" "dedicated" {
  cluster_id = cockroach_cluster.dedicated.id
  some_config_option = 1234
}
fantapop commented 2 months ago

I guess another consideration is whether we'd expose some of the outputs as part of the resource. I see that the api response has the following data:

PrometheusMetricExportInfo
{
    cluster_id*: string
    status: enum  // Allowed: NOT_DEPLOYED┃DISABLING┃ENABLING┃ENABLED┃ERROR
    targets: {   // targets is a map of ports exposing metrics to regions.
        [any-key]: string
    }
    user_message: string
}
aa-joshi commented 2 months ago

@aa-joshi @marksoper considering that there is no actual configuration for enabling prometheus what you do think of having it as an additional field on the cluster resource? It could be something like:

resource "cockroach_cluster" "dedicated" {
  name           = "cockroach-dedicated"
  cloud_provider = "GCP"
  prometheus_metric_export_enabled = true
  dedicated = {
  ....
  }

If we were planning to add configuration options to this at some point in the near future, it seems like keeping it as its own resource would be desirable but even in that case we could just have that resource just represent the configuration and keep the enabled / disabled flag on cluster.

For example,

resource "cockroach_cluster" "dedicated" {
  name           = "cockroach-dedicated"
  cloud_provider = "GCP"
  prometheus_metric_export_enabled = true
  dedicated = {
  ....
  }
resource "cockroach_cluster_prometheus_export_config" "dedicated" {
  cluster_id = cockroach_cluster.dedicated.id
  some_config_option = 1234
}

We have existing metric export integration which has their own resources:

Hence we would like to keep it consistent with other metric export integrations. I will create a seperate backlog item to track integrations against the cluster resource.

aa-joshi commented 2 months ago

I guess another consideration is whether we'd expose some of the outputs as part of the resource. I see that the api response has the following data:

PrometheusMetricExportInfo
{
    cluster_id*: string
    status: enum  // Allowed: NOT_DEPLOYED┃DISABLING┃ENABLING┃ENABLED┃ERROR
    targets: {   // targets is a map of ports exposing metrics to regions.
        [any-key]: string
    }
    user_message: string
}

Thanks for suggestion! I have made required changes to keep track of targets.

fantapop commented 2 months ago

Hence we would like to keep it consistent with other metric export integrations. I will create a seperate backlog item to track integrations against the cluster resource.

For future consideration, I think should always re-evaluate the best way to expose the api for any new resources. Existing resource patterns is of course valuable input into this decision but not the only consideration. In this case, since we want to expose the additional outputs, I agree that it should be a new resource.

arjunmahishi commented 2 months ago

Went over the changes in a pair programming session (learnt a lot about terraform lifecycle today!). This LGTM. Will wait for @fantapop's ✅

arjunmahishi commented 2 months ago

@fantapop Can you please give it one final look? Thanks!

arjunmahishi commented 1 month ago

@fantapop we have made the requested changes