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

[Feature Request]: grafana_data_source data allow non-existing datasource #1408

Closed
JustNZ opened this issue Mar 7, 2024 · 12 comments · Fixed by #1441
Closed

[Feature Request]: grafana_data_source data allow non-existing datasource #1408

JustNZ opened this issue Mar 7, 2024 · 12 comments · Fixed by #1441
Assignees

Comments

@JustNZ
Copy link

JustNZ commented Mar 7, 2024

Feature Request

Currently the data "grafana_data_source" returns error 404 if the datasource doesn't exist.
This is problematic for us since the tempo and loki datasource require one another on some feature flags.

Would it be possible to allow data "grafana_data_source" to work with non-existing datasources and don't let terraform fail if the datasource doesn't exist?

@julienduchesne
Copy link
Member

I think a grafana_data_sources resource would work here, right? This is usually the pattern that I see in providers: a strict single-ID resource and a listing resource for other scenarios

@JustNZ
Copy link
Author

JustNZ commented Mar 7, 2024

So currently our problem is this one here.

there is a race condition between the loki and tempo datasource

resource "grafana_data_source" "loki" {
  for_each = var.datasources.loki
  org_id   = var.org_id
  type     = "loki"
  name     = each.value.name
  url      = each.value.uri

  http_headers = each.value.headers

  json_data_encoded = jsonencode({
    tlsSkipVerify = each.value.tlsSkipVerify
    timeout       = each.value.timeout
    derivedFields = [
      {
        datasourceUid   = try(grafana_data_source.tempo[each.key].uid, null)
        matcherRegex    = ".*\\[(\\S*).*?\\]"
        name            = "traceID"
        url             = "$${__value.raw}"
        urlDisplayLabel = ""
      },
    ]
  })
}

resource "grafana_data_source" "tempo" {
  for_each = var.datasources.tempo
  org_id   = var.org_id
  type     = "tempo"
  name     = each.value.name
  url      = each.value.uri

  http_headers = each.value.headers

  json_data_encoded = jsonencode({
    tlsSkipVerify = each.value.tlsSkipVerify
    tracesToLogsV2 = {
      datasourceUid      = try(data.grafana_data_source.loki[each.key].uid, null)
      spanStartTimeShift = "-10m"
      spanEndTimeShift   = "10m"
      tags = [
        {
          key   = "instance",
          value = "instance"
        },
        {
          key   = "stage",
          value = "stage"
        },
        {
          key   = "host",
          value = "host"
        },
        {
          key   = "service.name"
          value = "service_name"
        }
      ]
      filterByTraceID = true
      filterBySpanID  = true
    }
    tracesToMetrics = {
      datasourceUid = try(grafana_data_source.prometheus[each.key].uid, null)
    }
    serviceMap = {
      datasourceUid = try(grafana_data_source.prometheus[each.key].uid, null)
    }
    search = each.value.search
    nodeGraph = {
      enabled = true
    }
    lokiSearch = {
      datasourceUid = try(data.grafana_data_source.loki[each.key].uid, null)
    }
  })
}

@julienduchesne
Copy link
Member

Gotcha. A listing datasource would work here then. No choice but to have a two-step apply though

@JustNZ
Copy link
Author

JustNZ commented Mar 15, 2024

Hey @julienduchesne ,
is such a listing datasource already implemented? wasn't able to find anything in the registry

@miljanic
Copy link
Contributor

miljanic commented Mar 16, 2024

Would it be possible to create a separate resource for data source configuration?
That way multiple mutually dependent data sources (like Loki and Tempo in the example above) could be created and configured in the same apply.

That resource could be called grafana_data_source_config and work in a way similar to this:
Note: I simplified the example from above for brevity.

resource "grafana_data_source" "tempo" {
  name = "tempo"
}

resource "grafana_data_source" "loki" {
  name = "loki"
}

resource "grafana_data_source_config" "tempo" {
  uid = grafana_data_source.tempo.uid
  json_data_encoded = jsonencode({
    # ...
    datasourceUid   = grafana_data_source.loki.uid
    # ...
  })
}

resource "grafana_data_source_config" "loki" {
  uid = grafana_data_source.loki.uid
  json_data_encoded = jsonencode({
    # ...
    datasourceUid   = grafana_data_source.tempo.uid
    # ...
  })
}

grafana_data_source - would be responsible for the creation of the data source via POST /api/datasources
grafana_data_source_config - would be responsible for the configuration via PUT /api/datasources/uid/:uid

@julienduchesne
Copy link
Member

Would it be possible to create a separate resource for data source configuration? That way multiple mutually dependent data sources (like Loki and Tempo in the example above) could be created and configured in the same apply.

Splitting the resource would be the only possible way to do this in a single apply, indeed. It does require a lot of changes to resources though and is prone to breaking existing functionality (unless they're two completely new resources, ds_without_config, ds_config)

@miljanic
Copy link
Contributor

It does not necessarily break the existing functionality. If there is a new resource only for the grafana_data_source_config, and the current grafana_data_source remains to function as it with no changes, a deprecation note in the documentation for the json_data_encoded field saying something like "This field is deprecated, use grafana_data_source_config resource instead" should be enough.

This approach would fix the multi-step apply issue (circular dependency) and not force any changes to the existing code using grafana provider.

As I can see, this approach is used by aws provider as well, here is an example of the S3 related configuration in one of the previous versions (link), and those deprecated fields are removed in the next major release.

@julienduchesne
Copy link
Member

Yeah, I get that. The part that's weird to me is that, unlike an S3 bucket, a Grafana datasource is non-functional without its config.

So the bare grafana_data_source resource would just not work on its own, which for a core resource, feels not OK.

@miljanic
Copy link
Contributor

Hmm, excellent point there!
Without any config, the data source resource is deployable but unusable.

However, that is not what I was suggesting. By adding the config resource and leaving the existing one unchanged, we enable the configuration through the data source resource (no need for an additional resource). On top of that, we also allow for a single-step deployment of examples such as the above.

What troubles me, is that anyone wanting to use vanilla Terraform to deploy a common configuration like Loki and Tempo logs and traces correlation is forced to change the whole pipeline by introducing additional deployment steps. I find it a red flag for the provider, unfortunately.

@julienduchesne
Copy link
Member

However, that is not what I was suggesting. By adding the config resource and leaving the existing one unchanged, we enable the configuration through the data source resource (no need for an additional resource). On top of that, we also allow for a single-step deployment of examples such as the above.

👍 Adding a new "config" resource and leaving the possibility to configure datasources as a single unit is probably what I'd do

@miljanic
Copy link
Contributor

Great!
What is the procedure for accepting this as a feature?

@julienduchesne
Copy link
Member

I am this project's dictator. It's accepted. I'd accept a PR for it, but can't promise an ETA otherwise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants