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 assets to the package spec #767

Merged
merged 9 commits into from
Jun 25, 2024
Merged

Conversation

mgiota
Copy link
Contributor

@mgiota mgiota commented Jun 24, 2024

Fixes https://github.com/elastic/observability-dev/issues/3602

✔️ Acceptance criteria

  • add SLO assets to the kibana spec
  • add slo test examples


- description: Folder containing Kibana SLO assets
type: folder
name: slo
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsoriano I have a draft PR here that adds the SLO assets to the kibana spec. I plan to add some unit tests in the PR, but here's what CI is complaining about at the moment:

could not load spec for "integration/kibana/spec.yml": could not parse folder specification file: yaml

I have a couple of questions:

  • Could you further assist why I am getting this error? I gave the name slo to the new folder. Shall I specify somewhere else what is considered a valid folder name?
  • Where shall I write my unit tests?
  • I was reading the Contributing guide. Shall I create a new Change Proposal issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CI is complaining

It turns out it was an indentation issue, which was fixed in this commit

Where shall I write my unit tests?

I added following slo examples:

  • test/packages/good/kibana/slo/good-slo-abc-1.json
  • test/packages/good_v2/kibana/slo/good_v2-slo-abc-1.json
  • test/packages/good_v3/kibana/slo/good_v3-slo-abc-1.json

Shall I create a new Change Proposal issue?

I didn't create any new issue, cause I found this one.

Choose a reason for hiding this comment

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

@mgiota - Do we need to load the SLO's inside the Kibana directory? Are there any guideline on defining SLO's.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@muthu-mps I added it here as per @jsoriano 's recommendation in the past. Here's the public SLO API and the SLO Elastic documentation

In this PR I added some examples of SLO creation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this commit I updated the README file to include the SLO assets. As you can see there are many other Kibana assets in the list as for example rules, ml modules, dashboards and I guess it makes sense to add slo here as well.

@mgiota mgiota requested a review from jsoriano June 24, 2024 11:58
@mgiota mgiota marked this pull request as ready for review June 24, 2024 22:11
@mgiota mgiota requested a review from a team as a code owner June 24, 2024 22:11
@mgiota
Copy link
Contributor Author

mgiota commented Jun 24, 2024

@jsoriano I open up this PR for review. Let me know what other changes I might need to do that I am not aware of. Thanks!

@mgiota
Copy link
Contributor Author

mgiota commented Jun 24, 2024

test integrations

@elasticmachine
Copy link

Created or updated PR in integrations repository to test this version. Check elastic/integrations#10231

@mgiota
Copy link
Contributor Author

mgiota commented Jun 24, 2024

I was following the Contributing guide and I commented test integrations in this PR, but I am not sure why the auto generated PR is failing. Could you help me out here?

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Change LGTM, but we have to decide on what version we would release it, and add a changelog entry.

To decide what version to use, the main question would be regarding the level of support in Kibana. If a package using this feature requires changes in Kibana to work properly, then we should add the change into 3.3.0-next, if not, we can add it in 3.2.1-next.

We release almost everything on patch versions, we use minors for changes that require also changes in Kibana, and majors for breaking or significant changes in the spec itself.

spec/integration/kibana/spec.yml Show resolved Hide resolved
@jsoriano
Copy link
Member

I am not sure why the auto generated PR is failing. Could you help me out here?

The failures there are not related to this change, no worries 🙂 aws package uses to be flaky, and azure_blob_storage has a known issue.

@mgiota
Copy link
Contributor Author

mgiota commented Jun 25, 2024

To decide what version to use, the main question would be regarding the level of support in Kibana. If a package using this feature requires changes in Kibana to work properly

As far as I understand there will be some changes in the Fleet module in kibana to make the whole integration work. So I guess we should add the change into 3.3.0-next.

add a changelog entry

I found a couple of changelog files. Where exactly should we add the new SLO asset support? Or is the change you recommnded in the spec file enough?

Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>
@mgiota
Copy link
Contributor Author

mgiota commented Jun 25, 2024

@jsoriano Now I am getting some validation errors:

found 1 validation error:
1. item [slo] is not allowed in folder [../../../../test/packages/good_v2/kibana]        	            	   

Do I need to specify the new slo item anywhere else? The tests started failing after I committed the change with the patch version. So I guess I am missing something else as well.

@jsoriano
Copy link
Member

Do I need to specify the new slo item anywhere else? The tests started failing after I committed the change with the patch version. So I guess I am missing something else as well.

The problem is that the good package uses an old version of the spec, and packages with SLOs will need to have at least a format_version equal or greater than 3.3.0.

I would recommend to move the test files you added to the good_v3 package and increase its format_version to 3.3.0.

You can run these tests locally for faster feedback, with go test ./... from the root directory of the repository.

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Thanks!

@elasticmachine
Copy link

💚 Build Succeeded

History

@muthu-mps
Copy link

@mgiota , @jsoriano - Just FYI, The directory structure we follow for the integrations to include SLO's and the general guidelines around using the ID's as name of the json file is still in discussion. We can continue those discussions in the issue. This may evolve going forward.

@mgiota
Copy link
Contributor Author

mgiota commented Jun 25, 2024

@jsoriano Looks like I am not authorized to merge this PR. Could you either merge the PR or grant me access to merge it?

Screenshot 2024-06-25 at 14 57 19

@jsoriano jsoriano merged commit 6247494 into elastic:main Jun 25, 2024
3 checks passed
@ruflin
Copy link
Member

ruflin commented Jun 25, 2024

@mgiota Thank you for making this happen!!!

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.

5 participants