feat: add metrics enablement #319

kccox · 2024-10-21T16:38:32Z

Event Streams allows enablement of enhanced customer metrics using the provision parameters. Add the metrics enablement to the main module, and to examples/complete, modules/fscloud, and solutions/quickstart. Update READMEs.

Description

Add metrics enablement to the module. This is a customer-requested feature.

Metrics enablement is already a provision parameter, so no update to the terraform provider version is needed.

Release required?

No release
Patch release (x.x.X)
Minor release (x.X.x)
Major release (X.x.x)

Run the pipeline

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

If relevant, a test for the change is included or updated with this PR.
If relevant, documentation for the change is included or updated with this PR.

For mergers

Use a conventional commit message to set the release level. Follow the guidelines.
Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
Use the Squash and merge option.

kccox · 2024-10-21T16:39:01Z

/run pipeline

github-actions · 2024-10-21T16:39:12Z

@kccox you must be in the team github-collaborators in order to trigger the pipeline.

Ak-sky · 2024-10-21T16:40:59Z

/run pipeline

Ak-sky · 2024-10-22T12:55:09Z

/run pipeline

kccox · 2024-10-22T13:55:10Z

@Ak-sky The CI failure was from invalid credentials while creating the topics:

kafka server: SASL Authentication failed: Authentication failed, invalid credentials

This is odd, since the same credentials were used to create the Event Streams service instance and that worked. Topic creation also worked in the previous CI run.

I looked at the logging from the kafka servers, but unfortunately Cloud Logs is having problems in us-south and they're unavailable. I also notice terraform created the testacc-subnet and VPC in eu-de, but the Event Streams instance in us-south - I don't know if that might be the cause.

Could you re-run the pipeline to see if the same error happens again?

Ak-sky · 2024-10-22T14:11:21Z

/run pipeline

Ak-sky · 2024-10-23T08:28:09Z

/run pipeline

Ak-sky · 2024-10-23T08:28:56Z

@kccox, can you please point me to the terraform provider doc URL to support metrics?

kccox · 2024-10-23T15:09:19Z

@Ak-sky https://github.com/IBM-Cloud/terraform-provider-ibm/blob/master/examples/ibm-event-streams/README.md shows use of "metrics". We have tested provisioning with metrics using the terraform provider and it works.

The metrics are one of the provision parameters, so are in the "parameters" or "parameters_json" property of the ibm_resource_instance terraform resource.

https://github.com/IBM-Cloud/terraform-provider-ibm/blob/master/website/docs/r/resource_instance.html.markdown -- see example for Cloud Logs for "parameters" use

However I don't think it's these code changes that are causing the problem. The infrastructure test for #321 failed in the same way, with SASL Authentication failed: Authentication failed, invalid credentials responses from the servers. This indicates that the API key used by terraform doesn't have admin authorization for the Event Streams service instance. Has the key, or any related policies, changed lately?

EDIT: I see that pull 321 is to fix https://github.ibm.com/GoldenEye/issues/issues/11171, reporting that the main module tests started failing a week ago. Unfortunately I don't have access to that pipeline instance or the COS bucket with the logs.

kccox · 2024-10-23T21:22:50Z

I have converted this to draft because we would prefer to implement #322 first. That will allow the metrics variable to be list(string) and validated.

ocofaigh · 2024-10-29T11:24:20Z

@kccox FYI, looks like a PR has been raised for migrating to parameters_json -> #325

ocofaigh · 2024-10-30T17:06:36Z

@kccox #325 is merged now so you should be good to proceed to use parameters_json to add this support

kccox · 2024-10-30T18:58:54Z

/run pipeline

kccox · 2024-10-30T19:08:24Z

@ocofaigh This is ready for review

ocofaigh · 2024-11-04T20:32:38Z

examples/complete/variables.tf

@@ -105,3 +105,9 @@ variable "topics" {
    }
  ]
 }
+
+variable "metrics" {


please don'e expose this as a variable in the example. We don't wan't to make examples flexible solutions for people to use. Instead I suggest you hard code metrics in the example main.tf and add all the allowed values so our our tests test the feature

examples/complete is using the default standard plan, and the enhanced metrics are only available for enterprise (https://cloud.ibm.com/docs/EventStreams?topic=EventStreams-metrics). However I will hard-code metrics: [] in the main.tf and remove it from the variables.tf.

Editing examples/complete/variables.tf, I am now confused. There are variables for several other module properties (resource tags, access tags, schemas, topics) which are used in main.tf. Why would metrics be treated differently?

schemas = var.schemas tags = var.resource_tags access_tags = var.access_tags topics = var.topics

ah they should not be exposed in the example. I'll create a separate issue for the core team to clean that up, We don't want people to start consuming the examples, as they not intended to be consumable solutions - just reference on how the module can be used.

created tracking issue #331

examples/fscloud/variables.tf

ibm_catalog.json

ocofaigh

See comments

ocofaigh · 2024-11-04T22:39:12Z

examples/fscloud/main.tf

@@ -62,6 +62,7 @@ module "event_streams" {
  tags                       = var.resource_tags
  topics                     = var.topics
  existing_kms_instance_guid = var.existing_kms_instance_guid
+  metrics                    = []


can you add the allowed values here so they get tested in the tests we run?

examples/fscloud doesn't set the plan to enterprise-3nodes-2tb, so is using a standard plan, and these options are not available for standard. Checking our provision code, I see that they're just ignored, and don't cause an error - but I can't promise it will always work that way.

So I could include the allowed values, but it would only be a test of the terraform validator, and might break in the future if we improve the parameters checking.

The fscloud example is calling the modules/fscloud submodule, and this is hard code to use the "enterprise-3nodes-2tb" plan

examples/complete/main.tf

kccox · 2024-11-07T22:27:01Z

@ocofaigh Added the metrics to examples/fscloud

ocofaigh

see remaining comments

variables.tf

solutions/quickstart/variables.tf

ibm_catalog.json

ocofaigh · 2024-11-11T17:06:48Z

/run pipeline

terraform-ibm-modules-ops · 2024-11-12T11:55:22Z

🎉 This PR is included in version 2.8.0 🎉

The release is available on:

GitHub release
v2.8.0

Your semantic-release bot 📦🚀

kccox requested review from Ak-sky and akocbek as code owners October 21, 2024 16:38

Ak-sky changed the title ~~chore: add metrics enablement~~ feat: add metrics enablement Oct 22, 2024

kccox marked this pull request as draft October 23, 2024 21:19

add metrics

a71027e

kccox force-pushed the add-metrics branch from f57fea3 to a71027e Compare October 30, 2024 18:42

kccox marked this pull request as ready for review October 30, 2024 18:52

Merge branch 'main' into add-metrics

7a30674

ocofaigh reviewed Nov 4, 2024

View reviewed changes

examples/fscloud/variables.tf Outdated Show resolved Hide resolved

ocofaigh reviewed Nov 4, 2024

View reviewed changes

ibm_catalog.json Outdated Show resolved Hide resolved

ocofaigh requested changes Nov 4, 2024

View reviewed changes

remove metrics variables from examples

f093522

ocofaigh reviewed Nov 4, 2024

View reviewed changes

examples/complete/main.tf Show resolved Hide resolved

add metrics values to fscloud example

bab0be7

ocofaigh requested changes Nov 8, 2024

View reviewed changes

variables.tf Outdated Show resolved Hide resolved

solutions/quickstart/variables.tf Outdated Show resolved Hide resolved

kccox added 2 commits November 8, 2024 09:07

remove metrics from quickstart

4d6d185

add check that metrics are only used with enterprise plan

19be2b8

kccox force-pushed the add-metrics branch from 793924c to 19be2b8 Compare November 8, 2024 15:30

ocofaigh reviewed Nov 8, 2024

View reviewed changes

ibm_catalog.json Outdated Show resolved Hide resolved

remove metrics from catalog

b55463d

ocofaigh approved these changes Nov 11, 2024

View reviewed changes

Merge branch 'main' into add-metrics

fa4c26d

ocofaigh merged commit 097ca90 into terraform-ibm-modules:main Nov 12, 2024
2 checks passed

terraform-ibm-modules-ops added the released label Nov 12, 2024

kccox deleted the add-metrics branch November 15, 2024 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add metrics enablement #319

feat: add metrics enablement #319

kccox commented Oct 21, 2024 •

edited by ocofaigh

Loading

kccox commented Oct 21, 2024

github-actions bot commented Oct 21, 2024

Ak-sky commented Oct 21, 2024

Ak-sky commented Oct 22, 2024

kccox commented Oct 22, 2024

Ak-sky commented Oct 22, 2024

Ak-sky commented Oct 23, 2024

Ak-sky commented Oct 23, 2024

kccox commented Oct 23, 2024 •

edited

Loading

kccox commented Oct 23, 2024

ocofaigh commented Oct 29, 2024

ocofaigh commented Oct 30, 2024

kccox commented Oct 30, 2024

kccox commented Oct 30, 2024

ocofaigh Nov 4, 2024

kccox Nov 4, 2024

kccox Nov 4, 2024 •

edited

Loading

ocofaigh Nov 4, 2024

ocofaigh Nov 4, 2024

ocofaigh left a comment

ocofaigh Nov 4, 2024

kccox Nov 5, 2024

ocofaigh Nov 7, 2024

kccox commented Nov 7, 2024

ocofaigh left a comment

ocofaigh commented Nov 11, 2024

terraform-ibm-modules-ops commented Nov 12, 2024

feat: add metrics enablement #319

feat: add metrics enablement #319

Conversation

kccox commented Oct 21, 2024 • edited by ocofaigh Loading

Description

Release required?

Run the pipeline

Checklist for reviewers

For mergers

kccox commented Oct 21, 2024

github-actions bot commented Oct 21, 2024

Ak-sky commented Oct 21, 2024

Ak-sky commented Oct 22, 2024

kccox commented Oct 22, 2024

Ak-sky commented Oct 22, 2024

Ak-sky commented Oct 23, 2024

Ak-sky commented Oct 23, 2024

kccox commented Oct 23, 2024 • edited Loading

kccox commented Oct 23, 2024

ocofaigh commented Oct 29, 2024

ocofaigh commented Oct 30, 2024

kccox commented Oct 30, 2024

kccox commented Oct 30, 2024

ocofaigh Nov 4, 2024

Choose a reason for hiding this comment

kccox Nov 4, 2024

Choose a reason for hiding this comment

kccox Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

ocofaigh Nov 4, 2024

Choose a reason for hiding this comment

ocofaigh Nov 4, 2024

Choose a reason for hiding this comment

ocofaigh left a comment

Choose a reason for hiding this comment

ocofaigh Nov 4, 2024

Choose a reason for hiding this comment

kccox Nov 5, 2024

Choose a reason for hiding this comment

ocofaigh Nov 7, 2024

Choose a reason for hiding this comment

kccox commented Nov 7, 2024

ocofaigh left a comment

Choose a reason for hiding this comment

ocofaigh commented Nov 11, 2024

terraform-ibm-modules-ops commented Nov 12, 2024

kccox commented Oct 21, 2024 •

edited by ocofaigh

Loading

kccox commented Oct 23, 2024 •

edited

Loading

kccox Nov 4, 2024 •

edited

Loading