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

Update stack monitoring spec 3.x #12156

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jsoriano
Copy link
Member

Migrate stack monitoring packages to spec 3.x.

@jsoriano jsoriano self-assigned this Dec 18, 2024
@andrewkroh andrewkroh added dashboard Relates to a Kibana dashboard bug, enhancement, or modification. Integration:beat Beat Integration:elasticsearch Elasticsearch Integration:kibana Kibana labels Dec 18, 2024
@elastic-vault-github-plugin-prod
Copy link

elastic-vault-github-plugin-prod bot commented Dec 18, 2024

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@consulthys
Copy link
Contributor

@jsoriano Thanks for initiating this. Quick noob question: why did you use 3.0.4 instead of 3.3.1 as you commented here?

@jsoriano
Copy link
Member Author

why did you use 3.0.4 instead of 3.3.1 as you commented here?

This package is intended for versions of kibana over 8.10.2. Kibana till 8.16 was filtering out packages up to 3.0 versions.
In summary, for packages supporting versions of kibana below 8.16, it is safer to use 3.0.

@jsoriano jsoriano mentioned this pull request Dec 19, 2024
4 tasks
@consulthys
Copy link
Contributor

Thanks @jsoriano

Kibana till 8.16 was filtering out packages up to 3.0 versions.

Where can I learn about such Kibana-specific constraints?

@@ -32,7 +32,7 @@
type: integer
metric_type: gauge
- name: read_exceptions.exception
type: object
type: flattened
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason for switching to flattened is because otherwise object_type would be required?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, type: object alone is not allowed because it is like an empty mapping.

I could not find an example document with this field, so I have chosen flattened as a quite safe option. But it would be indeed better to provide a more specific mapping. Do you know what is the structure of these fields?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can actually find a sample event containing some data for that field here, which only contains two subfields, one keyword and one text, which I guess rules out the object_type option altogether:

                      "read_exceptions": {
                        "type": "nested",
                        "properties": {
                          "exception": {
                            "type": "object",
                            "properties": {
                              "reason": {
                                "type": "text"
                              },
                              "type": {
                                "type": "keyword"
                              }
                            }
                          },

Also interesting is to see how that field is actually used in Stack Monitoring, i.e. the query only checks for the existence of the exceptions object (which flattened also supports), and then the type subfield is read.

I'll do some more checks to see if changing from object to flattened doesn't break anything (also in the case of monitoring indices not having the same mapping after rollover)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, these mappings with nested sound good too, I can try with them.

@consulthys
Copy link
Contributor

As a result of seeing the output of elastic-package build, all the changes make sense (except the flattened one I flagged)

@jsoriano
Copy link
Member Author

Kibana till 8.16 was filtering out packages up to 3.0 versions.

Where can I learn about such Kibana-specific constraints?

At the moment this is only in the default kibana configs. We should probably document this somewhere else, similar to what we have for kibana constraints in https://github.com/elastic/elastic-package/blob/main/docs/howto/stack_version_support.md#when-to-update-the-condition.

@jsoriano
Copy link
Member Author

Adding guidelines for spec version to elastic/elastic-package#2291

elastic:
subscription: basic
kibana:
version: ^8.11.2
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the first safe version to use secrets. We can also delay the adoption of secrets, then I would revert the change in the kibana constraint, and set secret: false in the cases where it is set to true now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be fine bumping from 8.10.1 to 8.11.2

@jsoriano jsoriano marked this pull request as ready for review December 19, 2024 18:22
@jsoriano jsoriano requested a review from a team as a code owner December 19, 2024 18:22
@andrewkroh andrewkroh added the Team:Stack Monitoring Stack Monitoring team [elastic/stack-monitoring] label Dec 19, 2024
@@ -32,7 +32,19 @@
type: integer
metric_type: gauge
- name: read_exceptions.exception
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be just read_exceptions since exception is now a field of its own below?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, good catch, thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I hadn't noticed that there was already a definition from these fields. I think we only need to remove the definition of the read_exceptions.exception object.

packages/elasticsearch/docs/README.md Outdated Show resolved Hide resolved
packages/elasticsearch/docs/README.md Outdated Show resolved Hide resolved
packages/elasticsearch/docs/README.md Outdated Show resolved Hide resolved
packages/elasticsearch/docs/README.md Outdated Show resolved Hide resolved
kibana.version: ^8.10.2
elastic.subscription: basic
kibana:
version: ^8.10.2
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also probably be 8.11.2

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't updated the version here because this package doesn't make use of secrets, or afaik anything not available in 8.10.

@elasticmachine
Copy link

💚 Build Succeeded

History

cc @jsoriano

Copy link

Quality Gate failed Quality Gate failed

Failed conditions
64.9% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dashboard Relates to a Kibana dashboard bug, enhancement, or modification. Integration:beat Beat Integration:elasticsearch Elasticsearch Integration:kibana Kibana Team:Stack Monitoring Stack Monitoring team [elastic/stack-monitoring]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants