-
Notifications
You must be signed in to change notification settings - Fork 525
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
Switch apm integration package to input-only package #11529
Comments
We should take a staged approach to minimise risk:
In the end we should still have the integration installed for non-Serverless, but not in any Serverless environments. For steps 4 and 5, we can test locally in apm-server by adapting #12066. Some other things require further thought and discussion before going all the way with rolling this out to non-Serverless:
|
I'm not 100% sure about this now. If ILM overrides DSL then it's not breaking, will need to look into this some more. index.lifecycle.prefer_ilm (defaults to EDIT: confirmed that the existing instructions still work, even with DSL enabled. So, that's pretty great - we can make this change first and update the instructions later to cover both DSL (simple) and ILM (advanced). |
|
## Summary We will depend on the apm-data Elasticsearch plugin for setting up index templates and ingest pipelines. We have been testing this in serverless dev and QA with config overrides -- this is just final step to roll it out to all environments. See elastic/apm-server#11529
The apm-data plugin is enabled in all serverless environments, and disabling the integration package will roll out soon. |
@axw do we still need the grok processor to perform the version check in apm package ? |
@kruskall when the package is input-only we won't need or use the grok processor anymore |
The integration package is no longer installed in any of the serverless environments. We'll let this soak for a bit, but I think we can now look towards enabling the plugin by default in Elasticsearch and then removing the data stream definitions from the integration package - probably for 8.14+. |
## Summary We will depend on the apm-data Elasticsearch plugin for setting up index templates and ingest pipelines. We have been testing this in serverless dev and QA with config overrides -- this is just final step to roll it out to all environments. See elastic/apm-server#11529
I just opened #13181 to keep track of an undesired Kibana behavior when upgrading an integration package where data streams have been removed. This can be considered a blocker for step 5 mentioned in this comment as it would not mean removing the index template already created. |
An alternative to #13181 would be to increase the priority of the apm-data index templates over the integration package. Doesn't seem ideal, since we should be able to clean up those old index templates, but it's an option to unblock. |
@axw old index templates could be deleted by removing the integration though, but is it desirable? We already move forward with increasing the priorities in elastic/elasticsearch#108885.
What do you think? What is the versioning strategy for the plugin? If it's bundled with Elasticsearch (which is my understanding) we need to be careful if we break compatibility between integration index templates and plugin index templates (as the migration path above may not work). Also do removing the plugin removes index templates installed by it? |
This is not necessary, it's part of Elasticsearch and now enabled by default.
What's the reason for removing and reinstalling the integration? I think what we should do is:
We'll need to check that this rolls over the existing data streams, and starts using the new index templates.
It's part of ES, it doesn't have any specific versioning. Regarding backwards compatibility: a similar issue exists with integration packages, and the ingest pipelines are expected to "upgrade" data from older agents. We'll need to do this in the apm-data provided ingest pipeline too. That's why, for example, this part exists: https://github.com/elastic/elasticsearch/blob/7f35f1bed0a8f82466b3354bbd5935386839e311/x-pack/plugin/apm-data/src/main/resources/ingest-pipelines/metrics-apm.internal%40default-pipeline.yaml#L13-L27
You can't remove the plugin. You can disable it, but I'm pretty sure it won't delete the index templates. |
@axw thanks for the additional details! That clarifies
Do we expect users upgrading from older version to retain the integrations index templates forever? I see some additional burden for troubleshooting or avenues for bugs by doing that. As removing the integration would remove the assets the idea was to leverage that to clean up older index templates.
I'll proceed with testing this scenario. |
We shouldn't expect them to keep it, but we should expect that some users will not know they should uninstall it. So we should probably test both with and without uninstalling the integration package. |
Today I run this scenario:
and encountered this error:
I then uninstalled the integration and restarted the stack and tried ingesting data with |
@endorama this error occurs when a As a concrete example, for To fix this, we will need to add per interval mappings with the concrete values defined. The ES behavior for this was confirmed by ES engineers a while ago when we ran into the same problem with Fleet managed integrations. I suggest to also grep for any other |
To check if I understood: adding |
Opened elastic/elasticsearch#109043 to address the mentioned errors. |
While testing the upgrade path from 8.14 to 8.15 I noticed an [unexpected mapping error](elastic/apm-server#11529 (comment)). This PR aims at solving it by adding a concrete value to `metricset.interval` mappings as suggested [here](elastic/apm-server#11529 (comment)). Upon testing the upgrade path again the mentioned error did not present themselves anymore.
I prepared a PR to switch to input only package. There is a linter failure related to A last resort approach could be to add the smallest possible |
Upon suggestion in the channel I proceeded with adding the smallest |
@endorama since things are working as-is without explicitly changing the package type, maybe we could just call this done. Is there some user-impacting effect of changing the package type at the moment? |
That is my current conclusion as well,
This is not 100% clear to me and I'm trying to confirm a couple of hypothesis I have on input packages. I'm working to have more details soon & report back (hopefully today) |
I'm going to recap my findings so far, before moving forward further. TL;DR: the changes have further implications that what initially suggested by the scope of this task. As our current solution does not show any evident limitation I would suggest following the course suggested by @axw. In these days I worked to answer a set of questions:
Input packages do not create index templates until the policy template included in them is used. They can be used from Kibana UI as other packages with a small caveat: incidentally all our currently available input packages are in Beta (this is determined due to their manifest version < 1.0.0) so they are not going to be shown by Kibana UI unless the specific flag is selected in the left column. Documentation around I tried editing the custom integration form but wasn't able to reach a complete working solution (creating policies works, editing policies don't but also don't trigger debug statements so maybe my understanding of code flows is still wrong). In addition to this issue I'm also concerned that the custom onboarding UI may break but I wasn't able to confirm if it's using policy variables or not and if yes where. The only benefit I've been able to discover so far is the "lazy" creation of index templates, but with the current To answer my initial questions:
|
Thanks for the investigation and writeup @endorama . |
Sounds good, let's call this done then! Thank you for digging into it @endorama ❤️ |
I've cleaned up the open PR related to this. |
Migrate the APM integration package to become an input-only package (elastic/package-spec#319), meaning that the package would not define any data streams.
This can only be done once #11528 is implemented.
The text was updated successfully, but these errors were encountered: