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

Add account metrics resource #94

Merged

Conversation

ennyjfrick
Copy link
Contributor

What was changed

Hi again! This PR adds a new resource to configure account-level metrics. It allows enabling and disabling of an account's metrics endpoint, configuring the CA cert accepted by the metrics endpoint, and exporting the metrics endpoint URI once created.

Why?

The new resource is intended to enable a better experience for operators with certificate and observability infrastructure already in Terraform. Instead of needing to configure certificates and Prometheus scrapes for Temporal Cloud out-of-band, Temporal Cloud observability features can now slot nicely in with existing Terraform configuration.

Checklist

  1. How was this tested:
    Run acceptance tests

  2. Any docs updates needed?
    Docs generated!

@ennyjfrick ennyjfrick requested a review from a team as a code owner April 24, 2024 17:08
@swgillespie
Copy link
Collaborator

thank you for all of the hard work on these 🙏 I will check this out!

@anekkanti
Copy link
Member

@ennyjfrick thank you for your PR.

The protos/apis you are referring from the tcld repo are meant to be deprecated, and will be replaced by the once in api-cloud repo.
We have a proposed account metrics mgmt apis that we are working on. PR:temporalio/api-cloud#14. We hope to get this out soon.
Can we hold off on this PR and wait for the apis to be available?

@ennyjfrick
Copy link
Contributor Author

@anekkanti totally fine with me!

@ennyjfrick
Copy link
Contributor Author

@anekkanti hey! wondering if there was any update on the updated account management APIs

@swgillespie
Copy link
Collaborator

@anekkanti can you share any updates you have here?

@anekkanti
Copy link
Member

anekkanti commented Aug 21, 2024

@ennyjfrick @swgillespie
We finally have a path to unblock the above APIs, and i am working on it.
It will likely take us at least a few weeks to get the required APIs out for metrics mgmt.

@ennyjfrick
Copy link
Contributor Author

@swgillespie @anekkanti I just opened temporalio/api-go#181 to update the generated client with the account get & update operations, will work on redoing this PR so the metrics endpoint resources uses those changes.

@ennyjfrick ennyjfrick force-pushed the ejf/add-account-observability-resource branch 2 times, most recently from 71041da to acca221 Compare October 23, 2024 17:14
@ennyjfrick
Copy link
Contributor Author

@swgillespie ok, should be good to go and ready for review here.

Two things to note:

  1. This uses a commit off of main of go.temporal.io/api rather than a release
  2. The protos switched to using enum fields for a bunch of fields that were previously just strings; my PR uses the deprecated string fields whenever a proto field is set since converting everything to the enums is a bigger project.

@swgillespie
Copy link
Collaborator

@rohitgup14 @anekkanti can you check this out? ^

@anekkanti
Copy link
Member

@ennyjfrick Just merged the changes that moves to using the enums in the new apis.
Can you rebase to those changes in main. Thanks!

@ennyjfrick ennyjfrick force-pushed the ejf/add-account-observability-resource branch from 014570e to 9f7f783 Compare October 31, 2024 22:13
@ennyjfrick
Copy link
Contributor Author

@anekkanti all done!

}

// example endpoint that uses the CA cert generated in this example
resource "temporalcloud_metrics_endpoint" "terraform2" {
Copy link
Member

Choose a reason for hiding this comment

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

What happens when you create 2 of the temporalcloud_metrics_endpoint resources? Which one will be applied to the account.

Is there a way to make this resource singleton? Not even sure if terraform supports such concept.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anekkanti there's no terraform support for singletons explicitly unfortunately, although we could put in some logic that does not allow you to create the resource if the endpoint is already configured and add some annotations stating this is an account singleton; this is what I've seen recommended for similar situations.

Copy link
Member

@anekkanti anekkanti Nov 4, 2024

Choose a reason for hiding this comment

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

put in some logic that does not allow you to create the resource if the endpoint is already configured and add some annotations stating this is an account singleton

I definitely like this, it will elevate some of the confusion when someone accidentally creates multiple resources.

@anekkanti
Copy link
Member

LGTM, a few things to address:

  1. Add a check to the Create where the provider errors out when the cert is already set on the account.
  2. Fix lint errors.

@ennyjfrick ennyjfrick requested review from jlacefie and a team as code owners December 13, 2024 21:54
@ennyjfrick ennyjfrick force-pushed the ejf/add-account-observability-resource branch from a20904b to c551ed4 Compare December 13, 2024 21:55
@ennyjfrick
Copy link
Contributor Author

@anekkanti howdy! sorry for the delay, should be good to go now

@ennyjfrick
Copy link
Contributor Author

@swgillespie / @anekkanti following up

@swgillespie
Copy link
Collaborator

@anekkanti @captainbeardo @briankassouf FYI

@ennyjfrick ennyjfrick force-pushed the ejf/add-account-observability-resource branch from 50423ca to 76bf4bd Compare January 9, 2025 17:43
@ennyjfrick
Copy link
Contributor Author

@swgillespie @anekkanti think I need one of you to hit the merge button, it's greyed out for me

@swgillespie swgillespie enabled auto-merge (squash) January 9, 2025 17:59
@swgillespie swgillespie merged commit 53f2c78 into temporalio:main Jan 9, 2025
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants