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.

Sign up for GitHub

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

Jump to bottom

[Fleet] use lowercase dataset in template names #180887

Merged
merged 3 commits into from
Apr 17, 2024
Merged

[Fleet] use lowercase dataset in template names #180887

merged 3 commits into from
Apr 17, 2024

Conversation

juliaElastic
Copy link
Contributor

@juliaElastic juliaElastic commented Apr 16, 2024 •
edited

Summary

Closes #180877

See repro steps in linked issue. Using lowercase dataset name in template/pipeline names solves the issue with the upgrade.
Will do more testing to make sure the lowercase naming doesn't cause any issue with agent ingesting.

Verification:

  • Tested locally by using a custom log path /var/tmp/test.log with a few lines of logs on a multipass vm.
  • Enrolled an agent and verified that the data is going to the logs-test data stream.
  • The integration policy is created on Custom Logs version 1.1.2 with dataset:Test and upgraded to 2.3.1.
  • The component, index template and ingest pipeline is using a lowercase logs-test prefix.
image image image

Checklist

@juliaElastic juliaElastic added release_note:fix backport:prev-minor Backport to the previous minor version (i.e. one version back from main) labels Apr 16, 2024
@juliaElastic juliaElastic self-assigned this Apr 16, 2024
@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@juliaElastic
Copy link
Contributor Author

/ci

@juliaElastic
Copy link
Contributor Author

/ci

@juliaElastic juliaElastic marked this pull request as ready for review April 16, 2024 12:21
@juliaElastic juliaElastic requested a review from a team as a code owner April 16, 2024 12:21
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fleet 159.1KB 159.1KB +42.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @juliaElastic

@juliaElastic
Copy link
Contributor Author

I'm wondering if this could be considered a breaking change that the name of the templates are changing. However, having uppercase letters in index template/component template names never seemed to work in elasticsearch.

criamico
criamico reviewed Apr 17, 2024
View reviewed changes
x-pack/plugins/fleet/common/services/datastream_es_name.ts
@@ -16,7 +16,7 @@ export function getRegistryDataStreamAssetBaseName(dataStream: {
type: string;
hidden?: boolean;
}): string {
const baseName = `${dataStream.type}-${dataStream.dataset}`;
const baseName = `${dataStream.type}-${dataStream.dataset.toLowerCase()}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a question: if I understand correctly, those templates from now on will be forced to have lowercase names. Could there be any risk in forcing this change?

@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Apr 17, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@criamico
Copy link
Contributor

I'm wondering if this could be considered a breaking change that the name of the templates are changing.

Wondering the same, it probably never worked but I think that we should highlight that this is forced from now on. Maybe let it know to integrations stakeholders or add it to the release notes. There could be some edge case that we don't foresee and would discover down the line.

criamico
criamico approved these changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@criamico criamico left a comment

Choose a reason for hiding this comment

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

Added some comments but LGTM 🚢

@juliaElastic
Copy link
Contributor Author

Wondering the same, it probably never worked but I think that we should highlight that this is forced from now on. Maybe let it know to integrations stakeholders or add it to the release notes. There could be some edge case that we don't foresee and would discover down the line.

Yes, it will be included in the release notes as a fix. I can send out an email to package developers as well to let them know.
cc @kpollich

@juliaElastic juliaElastic removed the backport:prev-minor Backport to the previous minor version (i.e. one version back from main) label Apr 17, 2024
@juliaElastic juliaElastic merged commit b4afac9 into elastic:main Apr 17, 2024
20 checks passed
@kibanamachine kibanamachine added v8.14.0 backport:skip This commit does not require backporting labels Apr 17, 2024
@juliaElastic juliaElastic added backport:prev-minor Backport to the previous minor version (i.e. one version back from main) and removed backport:skip This commit does not require backporting labels Apr 25, 2024
juliaElastic added a commit to juliaElastic/kibana that referenced this pull request Apr 25, 2024
@juliaElastic
[Fleet] use lowercase dataset in template names (elastic#180887)
8c1a1da 
## Summary

Closes elastic#180877

See repro steps in linked issue. Using lowercase dataset name in
template/pipeline names solves the issue with the upgrade.
Will do more testing to make sure the lowercase naming doesn't cause any
issue with agent ingesting.

Verification:
- Tested locally by using a custom log path `/var/tmp/test.log` with a
few lines of logs on a multipass vm.
- Enrolled an agent and verified that the data is going to the
`logs-test` data stream.
- The integration policy is created on Custom Logs version 1.1.2 with
`dataset:Test` and upgraded to 2.3.1.
- The component, index template and ingest pipeline is using a lowercase
`logs-test` prefix.

<img width="1787" alt="image"
src="https://github.com/elastic/kibana/assets/90178898/979d77d3-ba11-4d96-9870-5488c57d7aaf">

<img width="1033" alt="image"
src="https://github.com/elastic/kibana/assets/90178898/3c871462-0498-4b2f-836f-d4b84ac4cf46">

<img width="915" alt="image"
src="https://github.com/elastic/kibana/assets/90178898/e5d34264-650c-4313-b5f6-ff44b0af171d">



### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
@juliaElastic juliaElastic mentioned this pull request Apr 25, 2024
Merged
kpollich pushed a commit that referenced this pull request Apr 25, 2024
@juliaElastic
[8.13][Fleet] use lowercase dataset in template names (#180887) (#181670
7804244 
)

Backport #180887 to 8.13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@criamico criamico criamico approved these changes

Assignees

@juliaElastic juliaElastic

Labels
backport:prev-minor Backport to the previous minor version (i.e. one version back from main) release_note:fix Team:Fleet Team label for Observability Data Collection Fleet team v8.13.3 v8.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Fleet] Upgrading non-input type package to input type can break the installation
6 participants
@juliaElastic @apmmachine @kibana-ci @elasticmachine @criamico @kibanamachine