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 SLO support to provider #385

Merged
merged 47 commits into from
Aug 16, 2023
Merged

Conversation

wandergeek
Copy link
Contributor

@wandergeek wandergeek commented Jul 14, 2023

Overview

This PR adds kibana SLO management capabilities to the provider.

Testing

The custom metrics and histogram indicators are only supported in >8.10, so you will need to launch latest and greatest stack. I've been using kibana to do this:

Launch ES

	cd ~/work/kibana
	yarn kbn bootstrap
	yarn es snapshot --license trial --use-cached

Launch KB

	cd ~/work/kibana
	yarn kbn bootstrap
	yarn start --no-base-path

Run tests

export ELASTICSEARCH_ENDPOINTS=http://localhost:9200 \
KIBANA_ENDPOINT=http://localhost:5601 \
ELASTICSEARCH_USERNAME=elastic \
ELASTICSEARCH_PASSWORD=changeme

make lint test testacc docs-generate

@wandergeek wandergeek self-assigned this Jul 14, 2023
@wandergeek wandergeek changed the title Add SLO to support to provider Add SLO support to provider Jul 14, 2023
@tobio
Copy link
Member

tobio commented Jul 14, 2023

Other version specific resources define a SkipFunc to avoid the failing tests.

internal/clients/api_client.go Outdated Show resolved Hide resolved
internal/clients/api_client.go Show resolved Hide resolved
internal/clients/kibana/slo.go Outdated Show resolved Hide resolved
internal/clients/kibana/slo_test.go Outdated Show resolved Hide resolved
internal/clients/kibana/slo.go Show resolved Hide resolved
internal/kibana/slo.go Outdated Show resolved Hide resolved
internal/kibana/slo.go Outdated Show resolved Hide resolved
internal/kibana/slo.go Outdated Show resolved Hide resolved
internal/kibana/slo_test.go Show resolved Hide resolved
internal/kibana/slo_test.go Outdated Show resolved Hide resolved
@wandergeek
Copy link
Contributor Author

A note from testing to deal with later:

I restarted the stack, but still had some terraform state laying around. When I tried to do an apply, I got the following:

elasticstack_kibana_slo.test_slo: Refreshing state... [id=SozILMG9RtCl6hFyeubVow/7374c7b0-21e3-11ee-bd67-4fa609eac85f]
╷
│ Error: 404 Not Found
│
│   with elasticstack_kibana_slo.test_slo,
│   on provider.tf line 25, in resource "elasticstack_kibana_slo" "test_slo":
│   25: resource "elasticstack_kibana_slo" "test_slo" {
│
╵

A 404 is expected here since obviously that SLO is gone with the restart, but wondering if the provider should be smart enough to recreate it or just fail like this? I feel like at least the error message should be a bit more specific?

@wandergeek wandergeek marked this pull request as draft July 15, 2023 10:07
@tobio
Copy link
Member

tobio commented Jul 17, 2023

A 404 is expected here since obviously that SLO is gone with the restart, but wondering if the provider should be smart enough to recreate it or just fail like this? I feel like at least the error message should be a bit more specific?

The resource should be treating this error as if the resource no longer exists and needs to be recreated.

@dimuon
Copy link
Contributor

dimuon commented Aug 10, 2023

@wandergeek , please update CHANGELOG.md.

internal/kibana/slo.go Outdated Show resolved Hide resolved
internal/kibana/slo_test.go Outdated Show resolved Hide resolved
internal/kibana/slo_test.go Outdated Show resolved Hide resolved
@dimuon
Copy link
Contributor

dimuon commented Aug 10, 2023

@wandergeek , thanks for the contribution 👍
There are some nitpicks and few thoughts about adding support for 2 currently unsupported indicator types.

@wandergeek
Copy link
Contributor Author

Using a static spec file until elastic/kibana#161922 is merged.

@wandergeek , the PR is merged. Shall we switch to getting OpenAPI specs from Kibana repo instead of using the static file?

Yep, good idea. One thing I'm grappling with right now is whether to point the URL to main or a specific commit. Ideally I'd like to point it to a particular kibana version tag, but it looks like there's a number of commits that haven't been backported for some reason.

Curious what you think here. I've pointed it to main for now.

@dimuon
Copy link
Contributor

dimuon commented Aug 11, 2023

Using a static spec file until elastic/kibana#161922 is merged.

@wandergeek , the PR is merged. Shall we switch to getting OpenAPI specs from Kibana repo instead of using the static file?

Yep, good idea. One thing I'm grappling with right now is whether to point the URL to main or a specific commit. Ideally I'd like to point it to a particular kibana version tag, but it looks like there's a number of commits that haven't been backported for some reason.

Curious what you think here. I've pointed it to main for now.

I guess we can follow the same approach as for alerting.

@wandergeek
Copy link
Contributor Author

wandergeek commented Aug 14, 2023

I guess we can follow the same approach as for alerting.

This is what I'd like to do, but not all changes I want are in 8.9. I guess I could just point it at 8.10 even though its unreleased.

One thought that I had today was it'd be cool to somehow store/tag what sha the generated client is using, so when it gets updated, you can easily see what changed since the last update.

@wandergeek
Copy link
Contributor Author

wandergeek commented Aug 14, 2023

Ok, just pushed a mammoth update to this. I've broken up the indicators into their own subtypes as you suggested to work around the SDKv2 limitations. There's still a little clean up work to do, but thought I'd share so you could have a look overnight (if you have time).

@dimuon
Copy link
Contributor

dimuon commented Aug 14, 2023

@wandergeek , it looks good 👍

dimuon
dimuon previously approved these changes Aug 15, 2023
Copy link
Contributor

@dimuon dimuon left a comment

Choose a reason for hiding this comment

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

LGTM 👍

tobio
tobio previously approved these changes Aug 16, 2023
internal/clients/kibana/slo.go Outdated Show resolved Hide resolved
@wandergeek wandergeek merged commit 9be9f1f into elastic:main Aug 16, 2023
12 checks passed
jloleysens pushed a commit to jloleysens/terraform-provider-elasticstack that referenced this pull request Sep 11, 2023
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