provider/librato: Enable mgmt of Librato metrics #14562

wchrisjohnson · 2017-05-17T04:26:44Z

Update the Librato provider to allow the management of Librato metrics.
Updated the underlying go-librato vendored pkg to take advantage of recent changes that dovetail with these changes.
Refactored some common funcs out.

bernerdschaefer

I'm excited to see support for this!

bernerdschaefer · 2017-05-18T16:54:58Z

builtin/providers/librato/resource_librato_metric_test.go

+ composite = "s(\"librato.cpu.percent.user\", {\"environment\" : \"prod\", \"service\": \"api\"})"
+ attributes {
+ display_stacked = true,
+ created_by_ua = "go-librato/0.1"


I don't think anyone would ever write this attribute in a config; presumably it's set by go-librato when the metric is created. Does the resource behave as expected without this in the config?

The tests fail w/o this in the config. Perhaps I'm taking the wrong approach in fixing the issue:

hashicorp/terraform - [cj-add-librato-metrics●] » TF_LOG=info make testacc TEST=./builtin/providers/librato TESTARGS="-run=TestAccLibratoCompositeMetric" ==> Checking that code complies with gofmt requirements... go generate $(go list ./... | grep -v /terraform/vendor/) 2017/05/18 15:53:37 Generated command/internal_plugin_list.go TF_ACC=1 go test ./builtin/providers/librato -v -run=TestAccLibratoCompositeMetric -timeout 120m === RUN TestAccLibratoCompositeMetric 2017/05/18 15:53:41 [WARN] Test: Executing step 0 2017/05/18 15:53:41 [INFO] terraform: building graph: GraphTypeValidate 2017/05/18 15:53:41 [WARN] terraform: shadow graph disabled 2017/05/18 15:53:41 [INFO] terraform: building graph: GraphTypeRefresh 2017/05/18 15:53:41 [WARN] terraform: shadow graph disabled 2017/05/18 15:53:41 [INFO] terraform: building graph: GraphTypePlan 2017/05/18 15:53:41 [WARN] terraform: shadow graph disabled 2017/05/18 15:53:41 [WARN] Test: Step plan: DIFF: CREATE: librato_metric.foobar attributes.#: "0" => "1" attributes.0.display_stacked: "" => "true" composite: "" => "s(\"librato.cpu.percent.user\", {\"environment\" : \"prod\", \"service\": \"api\"})" description: "" => "A test composite metric" name: "" => "tftest-metric-x4dlskaqoh" type: "" => "composite" STATE: <no state> 2017/05/18 15:53:41 [INFO] terraform: building graph: GraphTypeApply 2017/05/18 15:53:41 [WARN] terraform: shadow graph disabled 2017/05/18 15:53:42 [INFO] terraform: building graph: GraphTypePlan 2017/05/18 15:53:42 [WARN] terraform: shadow graph disabled 2017/05/18 15:53:42 [INFO] terraform: building graph: GraphTypeRefresh 2017/05/18 15:53:42 [WARN] terraform: shadow graph disabled 2017/05/18 15:53:42 [INFO] Reading Librato Metric: tftest-metric-x4dlskaqoh 2017/05/18 15:53:42 [INFO] Read Librato Metric: {"name":"tftest-metric-x4dlskaqoh","description":"A test composite metric","type":"composite","composite":"s(\"librato.cpu.percent.user\", {\"environment\" : \"prod\", \"service\": \"api\"})","attributes":{"color":null,"display_max":null,"display_min":null,"display_units_long":"","display_units_short":"","display_stacked":true,"display_transform":"","created_by_ua":"go-librato/0.1"}} 2017/05/18 15:53:42 [INFO] terraform: building graph: GraphTypePlan 2017/05/18 15:53:42 [WARN] terraform: shadow graph disabled 2017/05/18 15:53:42 [WARN] Test: Executing destroy step 2017/05/18 15:53:42 [INFO] terraform: building graph: GraphTypeValidate 2017/05/18 15:53:42 [WARN] terraform: shadow graph disabled 2017/05/18 15:53:42 [INFO] terraform: building graph: GraphTypeRefresh 2017/05/18 15:53:42 [WARN] terraform: shadow graph disabled 2017/05/18 15:53:42 [INFO] Reading Librato Metric: tftest-metric-x4dlskaqoh 2017/05/18 15:53:42 [INFO] Read Librato Metric: {"name":"tftest-metric-x4dlskaqoh","description":"A test composite metric","type":"composite","composite":"s(\"librato.cpu.percent.user\", {\"environment\" : \"prod\", \"service\": \"api\"})","attributes":{"color":null,"display_max":null,"display_min":null,"display_units_long":"","display_units_short":"","display_stacked":true,"display_transform":"","created_by_ua":"go-librato/0.1"}} 2017/05/18 15:53:42 [INFO] terraform: building graph: GraphTypePlanDestroy 2017/05/18 15:53:42 [WARN] terraform: shadow graph disabled 2017/05/18 15:53:42 [WARN] Test: Step plan: DIFF: DESTROY: librato_metric.foobar STATE: librato_metric.foobar: ID = tftest-metric-x4dlskaqoh attributes.# = 1 attributes.0.aggregate = false attributes.0.color = attributes.0.created_by_ua = go-librato/0.1 attributes.0.display_max = attributes.0.display_min = attributes.0.display_stacked = true attributes.0.display_units_long = attributes.0.display_units_short = attributes.0.gap_detection = false composite = s("librato.cpu.percent.user", {"environment" : "prod", "service": "api"}) description = A test composite metric name = tftest-metric-x4dlskaqoh type = composite 2017/05/18 15:53:42 [INFO] terraform: building graph: GraphTypeApply 2017/05/18 15:53:42 [WARN] terraform: shadow graph disabled 2017/05/18 15:53:42 [INFO] Deleting Metric: tftest-metric-x4dlskaqoh 2017/05/18 15:53:42 [INFO] Verifying Metric tftest-metric-x4dlskaqoh deleted 2017/05/18 15:53:42 [INFO] GETing Metric tftest-metric-x4dlskaqoh 2017/05/18 15:53:42 [INFO] GET returned a 404 for Metric tftest-metric-x4dlskaqoh 2017/05/18 15:53:42 [INFO] I think Metric is deleted *** testAccCheckLibratoMetricDestroy ID: tftest-metric-x4dlskaqoh 2017/05/18 15:53:42 [INFO] terraform: building graph: GraphTypePlanDestroy 2017/05/18 15:53:42 [WARN] terraform: shadow graph disabled 2017/05/18 15:53:42 [INFO] terraform: building graph: GraphTypeRefresh 2017/05/18 15:53:42 [WARN] terraform: shadow graph disabled 2017/05/18 15:53:42 [INFO] terraform: building graph: GraphTypePlanDestroy 2017/05/18 15:53:42 [WARN] terraform: shadow graph disabled --- FAIL: TestAccLibratoCompositeMetric (0.78s) testing.go:280: Step 0 error: After applying this step and refreshing, the plan was not empty: DIFF: UPDATE: librato_metric.foobar attributes.0.created_by_ua: "go-librato/0.1" => "" STATE: librato_metric.foobar: ID = tftest-metric-x4dlskaqoh attributes.# = 1 attributes.0.aggregate = false attributes.0.color = attributes.0.created_by_ua = go-librato/0.1 attributes.0.display_max = attributes.0.display_min = attributes.0.display_stacked = true attributes.0.display_units_long = attributes.0.display_units_short = attributes.0.gap_detection = false composite = s("librato.cpu.percent.user", {"environment" : "prod", "service": "api"}) description = A test composite metric name = tftest-metric-x4dlskaqoh type = composite FAIL exit status 1 FAIL github.com/hashicorp/terraform/builtin/providers/librato 0.794s make: *** [testacc] Error 1

I think the created_by_ua attribute needs to be marked as Computed: true to signal that its value is set by the server, not by users.

That looks to be working!

bernerdschaefer · 2017-05-18T16:57:19Z

builtin/providers/librato/resource_librato_metric_test.go

+ }
+}
+
+func testAccCheckLibratoMetricType(metric *librato.Metric, validTypes []string) resource.TestCheckFunc {


It looks like this could be testAccCheckLibratoMetricType(metric *librato.Metric, wantType string) resource.TestCheckFunc and provide more valuable test coverage for the cases where it's used.

wchrisjohnson · 2017-05-22T18:30:32Z

@grubernaut @tombuildsstuff can I ping someone to review this, or can any Terraform or Librato engineer review/approve it? Excuse the ignorance... 😀

grubernaut

Left comments, nits, and suggestions. Happy to discuss any items further!
Also needs documentation for the new resource added. Thanks for the contribution!

grubernaut · 2017-05-22T18:58:37Z

builtin/providers/librato/common.go

+)
+
+// Encodes a hash into a JSON string
+func attributesFlatten(attrs map[string]string) (string, error) {


It looks like all of these common functions could be completely removed in favor of the ones inside the structure package.

NormalizeJsonString (https://github.com/hashicorp/terraform/blob/master/helper/structure/normalize_json.go#L8)
FlattenJsonToString (https://github.com/hashicorp/terraform/blob/master/helper/structure/flatten_json.go#L5)
ExpandJsonFromString (https://github.com/hashicorp/terraform/blob/master/helper/structure/expand_json.go#L5)

I've deleted this file for now. There is a decent amount of duplication within the Librato plugin (as well as the AWS plugin) that needs to be rectified, but that's a task for another PR. :-) . If I need these though, I'll use them vs the Librato specific ones so as to get us started with removing them.

grubernaut · 2017-05-22T19:00:06Z

builtin/providers/librato/resource_librato_metric.go

+)
+
+const (
+ metricTypeGauge = "gauge"


These constants aren't used anywhere... Is there a need for them?

If you need to validate the metric type, could also use the validation function StringInSlice, and pass in a slice of the valid metric types:

var validMetricTypes []string("gauge", "counter", "composite"} // Or use the constant types declared earlier // ... ValidateFunc: StringInSlice(validMetricTypes),

grubernaut · 2017-05-22T19:03:56Z

builtin/providers/librato/resource_librato_metric.go

+ Delete: resourceLibratoMetricDelete,
+
+ Schema: map[string]*schema.Schema{
+ "name": &schema.Schema{


nit: can remove the duplicate type declarations here:
~~&schema.Schema~~

wow, I see this ALL OVER the terraform plugin landscape... good to know!

grubernaut · 2017-05-22T19:06:21Z

builtin/providers/librato/resource_librato_metric.go

+ client := meta.(*librato.Client)
+
+ metric := new(librato.Metric)
+ if a, ok := d.GetOk("name"); ok {


name and type are required, so they won't ever be nil during the Create method. Can initialize the librato.Metric struct with these values

grubernaut · 2017-05-22T19:15:17Z

builtin/providers/librato/resource_librato_metric.go

+ metric.Description = librato.String(a.(string))
+ }
+ if a, ok := d.GetOk("period"); ok {
+ metric.Period = librato.Uint(a.(uint))


This will panic, as the interface a is an int and not a uint.
Need to cast it to an integer before an unsigned integer:

metric.Period = librato.Uint(uint(a.(int)))

grubernaut · 2017-05-22T20:17:56Z

builtin/providers/librato/resource_librato_metric_test.go

+ Check: resource.ComposeTestCheckFunc(
+ testAccCheckLibratoMetricExists("librato_metric.foobar", &metric),
+ testAccCheckLibratoMetricName(&metric, name),
+ testAccCheckLibratoMetricType(&metric, typ),


Test that the description was actually updated.

grubernaut · 2017-05-22T20:18:43Z

builtin/providers/librato/resource_librato_metric_test.go

+ Config: compositeMetricConfig(name, typ, fmt.Sprintf("An updated test %s metric", typ)),
+ Check: resource.ComposeTestCheckFunc(
+ testAccCheckLibratoMetricExists("librato_metric.foobar", &metric),
+ testAccCheckLibratoMetricName(&metric, name),


Test that the description was actually updated.

grubernaut · 2017-05-22T20:19:11Z

builtin/providers/librato/resource_librato_metric_test.go

+ continue
+ }
+
+ fmt.Printf("*** testAccCheckLibratoMetricDestroy ID: %s\n", rs.Primary.ID)


Use log.Printf("[DEBUG] ....")

grubernaut · 2017-05-22T20:20:05Z

builtin/providers/librato/resource_librato_service.go

@@ -37,49 +36,12 @@ func resourceLibratoService() *schema.Resource {
 "settings": &schema.Schema{
 Type: schema.TypeString,
 Required: true,
- StateFunc: normalizeJson,
+ StateFunc: normalizeJSON,


Can use the structure function here

removed for now

grubernaut · 2017-05-22T20:20:42Z

builtin/providers/librato/resource_librato_service.go

@@ -91,7 +53,7 @@ func resourceLibratoServiceCreate(d *schema.ResourceData, meta interface{}) erro
 service.Title = librato.String(v.(string))
 }
 if v, ok := d.GetOk("settings"); ok {
- res, err := resourceLibratoServicesExpandSettings(normalizeJson(v.(string)))
+ res, err := attributesExpand(normalizeJSON(v.(string)))


Can use the structure function for the rest of these

removed for now

wchrisjohnson · 2017-05-22T22:21:49Z

Thanks for the extensive review @grubernaut - I will dig in and get these comments addressed asap.

Much of the approach I took here was based on existing code in the Librato provider. COuld you point me toward another provider I could look at to get a better sense of best practices and good habits to embrace around terraform development?

grubernaut · 2017-05-22T22:45:07Z

@wchrisjohnson anytime! I would check out some of the resources in the AWS provider, the OPC provider, or the Fastly provider. The code here looks good, was just giving as much feedback and knowledge as I could. Happy to help further wherever needed!

grubernaut · 2017-05-25T13:02:37Z

Hey @wchrisjohnson, just remembered, we wrote a provider development guide that might help out as well: https://www.terraform.io/guides/writing-custom-terraform-providers.html.
Also happy to help if you have any further questions on either the guide, or any further clarification. Thanks!

wchrisjohnson · 2017-05-25T18:24:00Z

@grubernaut I think this PR is ready to be reviewed again. Thanks in advance!

wchrisjohnson · 2017-05-26T14:25:33Z

Could we please get another review of this PR? We'd very much like to start using this new module!

@stack72 @catsby

grubernaut

Thanks for this! LGTM!

wchrisjohnson · 2017-05-26T18:53:28Z

Thanks @grubernaut !!!

I have a quick question. I work for Heroku. Does terraform have any sort of program to give commit rights to a provider and its modules if they work for the provider? We are wondering if one or more of us can get commit rights for the Heroku provider? If so, how would we go about that?

stack72 · 2017-05-28T10:23:52Z

Chris Can you mail me at stack72 @ HashiCorp.com and we can chat about this :) P.

…

On Fri, 26 May 2017 at 21:53, Chris Johnson ***@***.***> wrote: Thanks @grubernaut <https://github.com/grubernaut> !!! I have a quick question. I work for Heroku. Does terraform have any sort of program to give commit rights to a provider and its modules if they work for the provider? We are wondering if one or more of us can get commit rights for the Heroku provider? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14562 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN5767xl4TehdkkNoTViIJNUTOyYZ5cks5r9x-rgaJpZM4NdXDI> .

ghost · 2020-04-12T01:53:59Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

wchrisjohnson changed the title ~~Enable mgmt of Librato metrics~~ Enable mgmt of Librato metrics *** WIP *** May 17, 2017

wchrisjohnson changed the title Enable mgmt of Librato metrics *** WIP *** provider/librato: Enable mgmt of Librato metrics *** WIP *** May 17, 2017

tombuildsstuff added enhancement provider/librato labels May 17, 2017

grubernaut added the new-resource label May 17, 2017

wchrisjohnson force-pushed the cj-add-librato-metrics branch from c379b6f to 519db10 Compare May 17, 2017 14:47

bernerdschaefer approved these changes May 18, 2017

View reviewed changes

wchrisjohnson changed the title provider/librato: Enable mgmt of Librato metrics *** WIP *** provider/librato: Enable mgmt of Librato metrics May 18, 2017

grubernaut suggested changes May 22, 2017

View reviewed changes

wchrisjohnson added 7 commits May 25, 2017 14:09

Enable CRUD of metrics

002c998

Fixup bad rebase

52040c6

Separate metric test by type

45c93cf

Cleanup and slight refactors

78b61ad

Add documentation for metrics

53a1a4b

Restore previous version for now

6a0dca9

Respond to review feedback

1c76194

wchrisjohnson force-pushed the cj-add-librato-metrics branch from 4add5bf to 1c76194 Compare May 25, 2017 18:10

reduce sleep time

e6b4dc9

removed unused constants

527d79a

grubernaut approved these changes May 26, 2017

View reviewed changes

grubernaut merged commit 840f974 into hashicorp:master May 26, 2017

wchrisjohnson deleted the cj-add-librato-metrics branch May 26, 2017 18:55

elblivion mentioned this pull request Jun 6, 2017

Librato Alerts fixes #15118

Merged

ghost locked and limited conversation to collaborators Apr 12, 2020

provider/librato: Enable mgmt of Librato metrics #14562

provider/librato: Enable mgmt of Librato metrics #14562

Conversation

wchrisjohnson commented May 17, 2017 • edited Loading

bernerdschaefer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchrisjohnson May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchrisjohnson commented May 22, 2017

grubernaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchrisjohnson commented May 22, 2017

grubernaut commented May 22, 2017

grubernaut commented May 25, 2017

wchrisjohnson commented May 25, 2017

wchrisjohnson commented May 26, 2017 • edited Loading

grubernaut left a comment

Choose a reason for hiding this comment

wchrisjohnson commented May 26, 2017 • edited Loading

stack72 commented May 28, 2017 via email

ghost commented Apr 12, 2020

wchrisjohnson commented May 17, 2017 •

edited

Loading

wchrisjohnson May 18, 2017 •

edited

Loading

wchrisjohnson commented May 26, 2017 •

edited

Loading

wchrisjohnson commented May 26, 2017 •

edited

Loading