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

[datadog_dashboard] Add support for text_formats in query table widget requests #2587

Merged
merged 11 commits into from
Oct 3, 2024

Conversation

hyungl
Copy link
Contributor

@hyungl hyungl commented Sep 19, 2024

Motivation

PAYP-27
Customer has asked for Terraform support of an existing feature: Query Table text formats. Corresponding api-spec pr.

Existing dashboard schema containing text_formats:

{
    "id": "",
    "title": "",
    "description": "Created using the Datadog provider in Terraform",
    ...
    "widgets": [
        {
            "definition": {
                "requests": [
                    {
                        "aggregator": "max",
                        "alias": "cpu user",
                        "cell_display_mode": [
                            "number"
                        ],
                        "limit": 25,
                        "order": "desc",
                        "q": "avg:system.cpu.user{account:prod} by {service, team}",
                        "text_formats": [
                            [
                                {
                                    "match": {
                                        "type": "is",
                                        "value": "test"
                                    },
                                    "palette": "black_on_light_yellow",
                                    "replace": {
                                        "type": "all",
                                        "with": "test"
                                    }
                                }
                            ],
                            [
                                {
                                    "match": {
                                        "type": "is",
                                        "value": "versus"
                                    }
                                }
                            ]
                        ]
                    }
                ],
                ...
            },
            "id": 
        }
    ],
   ...
}

Fields

match is an object containing fields type, value. replace is an object containing fields type, with, and substring. palette is a dropdown of color options. custom_bg_color and custom_fg_color are strings.

text_formats is a nested array of objects

Each column of values in a query table accounts for each inner array of text_formats. Within each inner array, each object is a separate rule that governs the column of data.

In the provider, we have to define text_formats as a nested array

In resource_datadog_dashboard.go, we have to define the schema like this:

"text_formats": {
			Type:     schema.TypeList,
			Optional: true,
			Elem: &schema.Schema{
				Type: schema.TypeList,
				Elem: &schema.Resource{
					Schema: map[string]*schema.Schema{
						"match": {
                                                  ...

This seems to work, at least in terms of using terraform to manage dashboards

This example dashboard:

resource "datadog_dashboard" "query_table_dashboard" {
	title         = "{{uniq}}"
	description   = "Created using the Datadog provider in Terraform"
	layout_type   = "ordered"

	widget {
		query_table_definition {
			title_size = "16"
			title = "this is hyung"
			title_align = "right"
			live_span = "1d"
			request {
				aggregator = "max"
				q = "avg:system.cpu.user{account:prod} by {service, team}"
				alias = "cpu user"
				limit = 25
				order = "desc"
				cell_display_mode = ["number"]
				text_formats = [
					[{
						match = {
							type = "is"
							value = "test"
						}
						palette = "black_on_light_yellow"
						replace = {
							type = "all"
							with = "test"
				        },
						custom_bg_color = null 
						custom_fg_color = null 
					}],
					[{
						match = {
							type = "is"
							value = "versus"
						}
						palette = null
						replace = null 
						custom_bg_color = null
						custom_fg_color = null
					},
                    {
                        match = {
                            type = "is"
                            value = "today"
                        }
                        palette = null
						replace = null 
						custom_bg_color = null
						custom_fg_color = null
                    }]
				]
			}
			has_search_bar = "auto"
		}
	}
}

saves a dashboard like this:

{
    "id": "8px-pfi-msw",
    "title": "{{uniq}}",
    "description": "Created using the Datadog provider in Terraform",
    "author_handle": "[email protected]",
    "author_name": "Hyung Lee",
    "layout_type": "ordered",
    "url": "/dashboard/8px-pfi-msw/uniq",
    "is_read_only": false,
    "template_variables": [],
    "widgets": [
        {
            "definition": {
                "has_search_bar": "auto",
                "requests": [
                    {
                        "aggregator": "max",
                        "alias": "cpu user",
                        "cell_display_mode": [
                            "number"
                        ],
                        "limit": 25,
                        "order": "desc",
                        "q": "avg:system.cpu.user{account:prod} by {service, team}",
                        "text_formats": [
                            [
                                {
                                    "match": {
                                        "type": "is",
                                        "value": "test"
                                    },
                                    "palette": "black_on_light_yellow",
                                    "replace": {
                                        "type": "all",
                                        "with": "test"
                                    }
                                }
                            ],
                            [
                                {
                                    "match": {
                                        "type": "is",
                                        "value": "versus"
                                    }
                                },
                                {
                                    "match": {
                                        "type": "is",
                                        "value": "today"
                                    }
                                }
                            ]
                        ]
                    }
                ],
                "time": {
                    "live_span": "1d"
                },
                "title": "this is hyung",
                "title_align": "right",
                "title_size": "16",
                "type": "query_table"
            },
            "id": 1229419269467444
        }
    ],
    "notify_list": [],
    "created_at": "2024-09-23T03:39:48.265448+00:00",
    "modified_at": "2024-09-23T04:18:29.538357+00:00",
    "experience_type": "default",
    "template_variable_presets": [],
    "tags": [],
    "restricted_roles": []
}

Terraform doesn't respect that fields are defined as optional. All fields need to be specified, and given null values if unused

If any of the fields are left out in the terraform config, we see during testing (
Testing command: RECORD=true TESTARGS="-run TestAccDatadogDashboardQueryTableWithTextFormats" op run -- make testall,
which runs this terraform command: terraform refresh -no-color -input=false -lock-timeout=0s -lock=true
)
this error:

Error: Incorrect attribute value type
...
Inappropriate value for attribute "text_formats": element 0: element 0:
attributes "custom_bg_color", "custom_fg_color", and "replace" are required.

This is happening when the test is trying to do terraform refresh of the config.

Possible causes

Using logs, confirmed that the Provider function in this repo does specify the new optional fields as optional.

Example logging

log.Println(fmt.Sprintf("custom_fg_color is optional %v", provider.ResourcesMap["datadog_dashboard"].SchemaFunc()["widget"].Elem.(*schema.Resource).Schema["query_table_definition"].Elem.(*schema.Resource).Schema["request"].Elem.(*schema.Resource).Schema["text_formats"].Elem.(*schema.Schema).Elem.(*schema.Resource).Schema["custom_fg_color"].Optional))

To see why terraform was complaining, I had to build terraform from source and set logs in the dependency hcl library to see what's happening when the terraform config and schema are used.

hcldec.AttrSpec instead of hcldec.BlockListSpec

The nodes of the terraform schema are handled as specs. Setting logs here confirmed that text_formats are handled as *hcldec.AttrSpec, not *hcldec.BlockListSpec. Maybe this is why the optionals aren't working correctly? Another field in QueryTable.Request is conditional_formats which is only one level of array nesting. That one is evaluated as a BlockListSpec and seems to handle Optional values correctly.

Invalid Plan that is ignored

[WARN]  Provider "registry.terraform.io/datadog/datadog" produced an invalid plan for datadog_dashboard.query_table_dashboard, but we are tolerating it because it is using the legacy plugin SDK.
    The following problems may be the cause of any confusing errors from downstream operations:
      - .notify_list: planned value cty.SetValEmpty(cty.String) for a non-computed attribute
      - .reflow_type: planned value cty.StringVal("") for a non-computed attribute
      - .is_read_only: planned value cty.False for a non-computed attribute
      - .restricted_roles: planned value cty.SetValEmpty(cty.String) for a non-computed attribute
      - .tags: planned value cty.ListValEmpty(cty.String) for a non-computed attribute
      - .widget[0].query_table_definition[0].request[0].text_formats: planned value cty.ListVal([]cty.Value{cty.ListVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{"custom_bg_color":cty.StringVal(""), "custom_fg_color":cty.StringVal(""), "match":cty.MapVal(map[string]cty.Value{"type":cty.StringVal("is"), "value":cty.StringVal("test")}), "palette":cty.NullVal(cty.String), "replace":cty.MapValEmpty(cty.String)})})}) does not match config value cty.ListVal([]cty.Value{cty.ListVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{"custom_bg_color":cty.NullVal(cty.String), "custom_fg_color":cty.NullVal(cty.String), "match":cty.MapVal(map[string]cty.Value{"type":cty.StringVal("is"), "value":cty.StringVal("test")}), "palette":cty.NullVal(cty.String), "replace":cty.NullVal(cty.Map(cty.String))})})}) nor prior value cty.ListVal([]cty.Value{cty.ListVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{"custom_bg_color":cty.StringVal(""), "custom_fg_color":cty.StringVal(""), "match":cty.MapVal(map[string]cty.Value{"type":cty.StringVal("is"), "value":cty.StringVal("test")}), "palette":cty.StringVal("black_on_light_yellow"), "replace":cty.MapVal(map[string]cty.Value{"type":cty.StringVal("all"), "with":cty.StringVal("test")})})}), cty.ListVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{"custom_bg_color":cty.StringVal(""), "custom_fg_color":cty.StringVal(""), "match":cty.MapVal(map[string]cty.Value{"type":cty.StringVal("is"), "value":cty.StringVal("versus")}), "palette":cty.StringVal(""), "replace":cty.MapValEmpty(cty.String)}), cty.ObjectVal(map[string]cty.Value{"custom_bg_color":cty.StringVal(""), "custom_fg_color":cty.StringVal(""), "match":cty.MapVal(map[string]cty.Value{"type":cty.StringVal("is"), "value":cty.StringVal("today")}), "palette":cty.StringVal(""), "replace":cty.MapValEmpty(cty.String)})})})

Seems terraform is being lenient with the difference between null and "". Maybe this is causing issues downstream?

Inner fields not Optional after decoding?

Printing out the spec that terraform hcl is using at this part of the code:

&{Name:text_formats Type:{typeImpl:{typeImplSigil:{} ElementTypeT:{typeImpl:{typeImplSigil:{} ElementTypeT:{typeImpl:{typeImplSigil:{} AttrTypes:map[custom_bg_color:{typeImpl:{typeImplSigil:{} Kind:83}} custom_fg_color:{typeImpl:{typeImplSigil:{} Kind:83}} match:{typeImpl:{typeImplSigil:{} ElementTypeT:{typeImpl:{typeImplSigil:{} Kind:83}}}} palette:{typeImpl:{typeImplSigil:{} Kind:83}} replace:{typeImpl:{typeImplSigil:{} ElementTypeT:{typeImpl:{typeImplSigil:{} Kind:83}}}}] AttrOptional:map[]}}}}}} Required:false}

Maybe it's not good that Required: false appears at the level of the entire object, but not for each nested field that is optional.

Conclusion

We decided to add a text_format key for each inner array.
Sample terraform config:

text_formats {
   text_format {          <----The change is to key the inner array
      match {
         type = "is",
         value = "test"
      }
      palette = "green_on_white"
   }
   text_format {          
      match {
         type = "is_not",
         value = "test"
      }
      palette = "green_on_white"
   }
}
text_formats {
  text_format{
      match {
         type = "is",
         value = "test"
      }
      palette = "red_on_white"
   }
}

Appendix - notes on testing

Since terraform-provider-datadog depends on terraform-plugin-testing which depends on terraform-exec, I replicated the test scenario by building terraform locally:

  1. git clone [email protected]:hashicorp/terraform.git
  2. Add logging
  3. go build -ldflags "-w -s -X 'github.com/hashicorp/terraform/version.dev=no'" -o bin/ .
  4. In terraform's go.mod:
replace github.com/hashicorp/hcl/v2 => /Users/hyung.lee/go/src/github.com/DataDog/hcl

replace github.com/zclconf/go-cty v1.15.0 => /Users/hyung.lee/go/src/github.com/DataDog/go-cty
  1. I put a dashboard.tf file containing the config I wanted to test within a folder
  2. $DATADOG_ROOT/terraform/bin/terraform refresh -no-color -input=false -lock-timeout=0s -lock=true

Terraform seems to scan the list of list of objects correctly. A section of code translates the text_formats config from tuple to list of list of object:

in: {{{{} [{{{} [{{{} map[custom_bg_color:{{{}}} custom_fg_color:{{{}}} match:{{{} map[type:{{{} 83}} value:{{{} 83}}] map[]}} palette:{{{}}} replace:{{{}}}] map[]}}]}}]}} [[map[custom_bg_color:<nil> custom_fg_color:<nil> match:map[type:is value:test] palette:<nil> replace:<nil>]]]}
in type name: tuple
want type name: list of list of object

@hyungl hyungl force-pushed the hyung.lee/payp-27 branch 2 times, most recently from ad9491d to de35b6d Compare September 19, 2024 10:28
@hyungl hyungl changed the title query table text formats (WIP) query table widget - add text formats to request (WIP) Sep 19, 2024
@hyungl hyungl changed the title query table widget - add text formats to request (WIP) [Query Table widget] - Add text formats to request (WIP) Sep 19, 2024
@hyungl hyungl force-pushed the hyung.lee/payp-27 branch 7 times, most recently from 2d81d3b to 2b0d728 Compare September 20, 2024 10:00
@hyungl hyungl force-pushed the hyung.lee/payp-27 branch 5 times, most recently from 63f7a44 to 5eef67b Compare September 23, 2024 05:22
@brtu
Copy link
Contributor

brtu commented Sep 23, 2024

What would the consequences if we change text_formats from a list of lists to just a list? I suspect that may be causing the issue with requiring those explicitly set null fields

if v, ok := terraformTextFormatRule["palette"].(string); ok && len(v) != 0 {
datadogTextFormatRule.SetPalette(datadogV1.TableWidgetTextFormatPalette(v))
} else {
datadogTextFormatRule.Palette = nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is needed, otherwise the default will be selected in place of empty.

brtu
brtu previously approved these changes Sep 26, 2024
Copy link
Contributor

@brtu brtu left a comment

Choose a reason for hiding this comment

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

LGTM!

@hyungl hyungl marked this pull request as ready for review September 26, 2024 17:03
@hyungl hyungl requested review from a team as code owners September 26, 2024 17:03
@hyungl hyungl changed the title [Query Table widget] - Add text formats to request (WIP) [resource_datadog_dashboard] - Add support for text_formats in query table widget requests Sep 26, 2024
@hyungl hyungl changed the title [resource_datadog_dashboard] - Add support for text_formats in query table widget requests [resource_datadog_dashboard] Add support for text_formats in query table widget requests Sep 26, 2024
@hyungl hyungl changed the title [resource_datadog_dashboard] Add support for text_formats in query table widget requests [datadog_dashboard] Add support for text_formats in query table widget requests Sep 26, 2024
brtu
brtu previously approved these changes Sep 26, 2024
@drichards-87
Copy link
Contributor

Created a Jira ticket for Docs Team review.

brett0000FF
brett0000FF previously approved these changes Sep 27, 2024
skarimo
skarimo previously approved these changes Oct 1, 2024
@hyungl hyungl dismissed stale reviews from skarimo, brett0000FF, and brtu via 77cf686 October 3, 2024 14:32
@hyungl hyungl force-pushed the hyung.lee/payp-27 branch from b694ec8 to 77cf686 Compare October 3, 2024 14:32
@skarimo skarimo merged commit e012111 into master Oct 3, 2024
11 checks passed
@skarimo skarimo deleted the hyung.lee/payp-27 branch October 3, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants