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

[Fleet] Slow image loading on 7.10 #83631

Closed
ph opened this issue Nov 18, 2020 · 4 comments · Fixed by #83680
Closed

[Fleet] Slow image loading on 7.10 #83631

ph opened this issue Nov 18, 2020 · 4 comments · Fixed by #83680
Assignees
Labels
Team:Fleet Team label for Observability Data Collection Fleet team v7.11.0

Comments

@ph
Copy link
Contributor

ph commented Nov 18, 2020

From @ruflin

I'm currently playing around with the 7.10 release and every time I load the integrations page, images are loaded with quite some delay. I checked the headers of one of the .svg and it looks as following.

ruflin

I'm comparing this with the cache headers from the CDN: https://epr.elastic.co/package/apache/0.2.6/img/logo_apache.svg where we have max-age=600.

I remember we had a similar discussion in the past and thought we fixed it. On my localhost, the images load very quickly but the cache headers are the same as in Cloud.

@ph ph added the Team:Fleet Team label for Observability Data Collection Fleet team label Nov 18, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@ph
Copy link
Contributor Author

ph commented Nov 18, 2020

@ruflin When you said localhost, do you mean everything local even the registry?

@ph ph added the v7.11.0 label Nov 18, 2020
@ruflin
Copy link
Contributor

ruflin commented Nov 18, 2020

@ph Good question. I'm not sure anymore if I run it from source (means public registry) or the elastic-package setup. But I think it should not matter.

@jfsiii
Copy link
Contributor

jfsiii commented Nov 18, 2020

Based on these tests, it seems this is default/expected behavior

it('does not allow responses to be cached by default', async () => {
const { server: innerServer, createRouter } = await server.setup(setupDeps);
const router = createRouter('/');
router.get({ path: '/', validate: false, options: {} }, (context, req, res) => res.ok());
await server.start();
await supertest(innerServer.listener)
.get('/')
.expect('Cache-Control', 'private, no-cache, no-store, must-revalidate');
});

but we can override it

it('allows individual responses override the default cache-control header', async () => {
const { server: innerServer, createRouter } = await server.setup(setupDeps);
const router = createRouter('/');
router.get({ path: '/', validate: false, options: {} }, (context, req, res) =>
res.ok({
headers: {
'Cache-Control': 'public, max-age=1200',
},
})
);
await server.start();
await supertest(innerServer.listener).get('/').expect('Cache-Control', 'public, max-age=1200');
});

I'll put up a PR soon

jfsiii pushed a commit that referenced this issue Nov 19, 2020
closes #83631 

### Problem
Assets are served with a `cache-control` header that prevents any caching
<img src="https://user-images.githubusercontent.com/640/99534379-517d2300-2975-11eb-8c05-4fb3f127c52b.png"/>

### Root cause
Likely from this code https://github.com/elastic/kibana/blob/2a365ff6329544465227e61141ded6fba8bb2c80/src/core/server/http/http_tools.ts#L40-L43

Also based on these tests, it seems this is default/expected behavior

https://github.com/elastic/kibana/blob/b3eefb97da8e712789b5c5d2eeae65c886ed8f64/src/core/server/http/integration_tests/router.test.ts#L510-L520

### Proposed solution
Set the header via the response handler as shown in this test:
https://github.com/elastic/kibana/blob/b3eefb97da8e712789b5c5d2eeae65c886ed8f64/src/core/server/http/integration_tests/router.test.ts#L522-L536

### This PR
If this registry response contains a `cache-control` header, that value is included in the EPM response as well

In `master`, which points to `epr-snapshot`
<img width="742" alt="Screen Shot 2020-11-18 at 12 33 47 PM" src="https://user-images.githubusercontent.com/57655/99568352-4fc75580-299d-11eb-962f-6ff28fa9510d.png">
which matches https://epr-snapshot.elastic.co/package/apache/0.2.6/img/logo_apache.svg

or using `epr.elastic.co`, 
<img width="781" alt="Screen Shot 2020-11-18 at 12 31 56 PM" src="https://user-images.githubusercontent.com/57655/99568350-4fc75580-299d-11eb-966e-f3489c13edb5.png">
which matches https://epr.elastic.co/package/apache/0.2.6/img/logo_apache.svg
chrisronline pushed a commit to chrisronline/kibana that referenced this issue Nov 19, 2020
closes elastic#83631 

### Problem
Assets are served with a `cache-control` header that prevents any caching
<img src="https://user-images.githubusercontent.com/640/99534379-517d2300-2975-11eb-8c05-4fb3f127c52b.png"/>

### Root cause
Likely from this code https://github.com/elastic/kibana/blob/2a365ff6329544465227e61141ded6fba8bb2c80/src/core/server/http/http_tools.ts#L40-L43

Also based on these tests, it seems this is default/expected behavior

https://github.com/elastic/kibana/blob/b3eefb97da8e712789b5c5d2eeae65c886ed8f64/src/core/server/http/integration_tests/router.test.ts#L510-L520

### Proposed solution
Set the header via the response handler as shown in this test:
https://github.com/elastic/kibana/blob/b3eefb97da8e712789b5c5d2eeae65c886ed8f64/src/core/server/http/integration_tests/router.test.ts#L522-L536

### This PR
If this registry response contains a `cache-control` header, that value is included in the EPM response as well

In `master`, which points to `epr-snapshot`
<img width="742" alt="Screen Shot 2020-11-18 at 12 33 47 PM" src="https://user-images.githubusercontent.com/57655/99568352-4fc75580-299d-11eb-962f-6ff28fa9510d.png">
which matches https://epr-snapshot.elastic.co/package/apache/0.2.6/img/logo_apache.svg

or using `epr.elastic.co`, 
<img width="781" alt="Screen Shot 2020-11-18 at 12 31 56 PM" src="https://user-images.githubusercontent.com/57655/99568350-4fc75580-299d-11eb-966e-f3489c13edb5.png">
which matches https://epr.elastic.co/package/apache/0.2.6/img/logo_apache.svg
jfsiii pushed a commit that referenced this issue Nov 24, 2020
closes #83631 

### Problem
Assets are served with a `cache-control` header that prevents any caching
<img src="https://user-images.githubusercontent.com/640/99534379-517d2300-2975-11eb-8c05-4fb3f127c52b.png"/>

### Root cause
Likely from this code https://github.com/elastic/kibana/blob/2a365ff6329544465227e61141ded6fba8bb2c80/src/core/server/http/http_tools.ts#L40-L43

Also based on these tests, it seems this is default/expected behavior

https://github.com/elastic/kibana/blob/b3eefb97da8e712789b5c5d2eeae65c886ed8f64/src/core/server/http/integration_tests/router.test.ts#L510-L520

### Proposed solution
Set the header via the response handler as shown in this test:
https://github.com/elastic/kibana/blob/b3eefb97da8e712789b5c5d2eeae65c886ed8f64/src/core/server/http/integration_tests/router.test.ts#L522-L536

### This PR
If this registry response contains a `cache-control` header, that value is included in the EPM response as well

In `master`, which points to `epr-snapshot`
<img width="742" alt="Screen Shot 2020-11-18 at 12 33 47 PM" src="https://user-images.githubusercontent.com/57655/99568352-4fc75580-299d-11eb-962f-6ff28fa9510d.png">
which matches https://epr-snapshot.elastic.co/package/apache/0.2.6/img/logo_apache.svg

or using `epr.elastic.co`, 
<img width="781" alt="Screen Shot 2020-11-18 at 12 31 56 PM" src="https://user-images.githubusercontent.com/57655/99568350-4fc75580-299d-11eb-966e-f3489c13edb5.png">
which matches https://epr.elastic.co/package/apache/0.2.6/img/logo_apache.svg
jfsiii pushed a commit that referenced this issue Dec 7, 2020
## Summary
Store package assets (from Registry or local upload) in Elasticsearch. Related to proposal [issue](#83426) & [document](https://docs.google.com/document/d/18XoS6CSl9UxxPPBt9LXuJngf1Jv-4tl3jY6l19U1yH8)

 * New `epm-packages-assets` saved objects are stored on `.kibana` index, like our existing saved object `epm-packages`
 * Asset id is uuid v5 based on the package name, package version & file path. See 1974324
 * Add a list of IDs of all the installed assets, to `epm-packages` saved object. Like the existing `installed_` properties.  [Example](https://github.com/elastic/kibana/pull/83391/files#diff-fa07cac51b6a49bf1e4824bc2250c9a77dac6c7d6b0a56020f559ef1ff9be25fR491-R512) from a test

<details><summary>Mapping for new Saved Object</summary>

https://github.com/elastic/kibana/blob/37f7b6ded747edb5cc487661b801c5e1c0a102a7/x-pack%2Fplugins%2Ffleet%2Fserver%2Fsaved_objects%2Findex.ts#L329-L339
</details>

<details><summary>Additional property on existing <code>epm-packages</code> Saved Object</summary>

https://github.com/elastic/kibana/blob/c4f27ab25715a225c710727a37f5f105364e2f72/x-pack/plugins/fleet/server/saved_objects/index.ts#L306-L312

 I don't think the saved object changes are strictly required. It can be removed without changing much about how things work

- Pros: 
      - Preserves accurate record of the assets added at installation time. Separates what assets are currently available for package-version from what was installed. They _should_ be the same, but things happen.
      - Avoids a query to get the installed assets before operating on them
- Cons:
      - size/noise? Could be tens or hundreds of ids
      - migration?
</details>

### More details

**When are saved objects added?**
During installation, after all other actions have succeeded, just before marking the save object as installed, we commit all the files from the package to ES

https://github.com/elastic/kibana/blob/37f7b6ded747edb5cc487661b801c5e1c0a102a7/x-pack%2Fplugins%2Ffleet%2Fserver%2Fservices%2Fepm%2Fpackages%2F_install_package.ts#L193-L198

**When are documents removed from the index?**

In the `removeInstallation` function which is called in response to a `DELETE /api/fleet/epm/packages/pkgkey`

https://github.com/elastic/kibana/blob/37f7b6ded747edb5cc487661b801c5e1c0a102a7/x-pack%2Fplugins%2Ffleet%2Fserver%2Fservices%2Fepm%2Fpackages%2Fremove.ts#L72

or a failed package (re-)installation

https://github.com/elastic/kibana/blob/bf068739acce044ac27902253e8fc31df621f081/x-pack%2Fplugins%2Ffleet%2Fserver%2Fservices%2Fepm%2Fpackages%2Finstall.ts#L145




**How are we using these assets?**
We're not, currently. Here's an example showing how we could update [`getFileHandler`](https://github.com/elastic/kibana/blob/514b50e4c2d7a3be79d77e73838ff57b6cf1304a/x-pack%2Fplugins%2Ffleet%2Fserver%2Froutes%2Fepm%2Fhandlers.ts#L101) to check for local assets before reaching out to the Registry if we wished. It's not DRY, but it does work

```typescript
const esDocRoot = `http://elastic:changeme@localhost:9200/${PACKAGE_ASSETS_INDEX_NAME}/_doc`;
const escapedDocId = encodeURIComponent(`${pkgName}-${pkgVersion}/${filePath}`);
const esRes = await fetch(`${esDocRoot}/${escapedDocId}`);
const esJson = await esRes.json();
if (esJson.found) {
  const asset: PackageAsset = esJson._source;
  const body = asset.data_utf8 || Buffer.from(asset.data_base64, 'base64');
  return response.ok({
    body,
    headers: {
      'content-type': asset.media_type,
      // should add our own `cache-control` header here
      // kibana default is prevents caching: `private, no-cache, no-store, must-revalidate`
      // #83631
    },
  });
}
```

### Checklist
_updated tests to include new saved object output, no tests added yet_
- [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
jfsiii pushed a commit that referenced this issue Dec 7, 2020
## Summary
Store package assets (from Registry or local upload) in Elasticsearch. Related to proposal [issue](#83426) & [document](https://docs.google.com/document/d/18XoS6CSl9UxxPPBt9LXuJngf1Jv-4tl3jY6l19U1yH8)

 * New `epm-packages-assets` saved objects are stored on `.kibana` index, like our existing saved object `epm-packages`
 * Asset id is uuid v5 based on the package name, package version & file path. See 1974324
 * Add a list of IDs of all the installed assets, to `epm-packages` saved object. Like the existing `installed_` properties.  [Example](https://github.com/elastic/kibana/pull/83391/files#diff-fa07cac51b6a49bf1e4824bc2250c9a77dac6c7d6b0a56020f559ef1ff9be25fR491-R512) from a test

<details><summary>Mapping for new Saved Object</summary>

https://github.com/elastic/kibana/blob/37f7b6ded747edb5cc487661b801c5e1c0a102a7/x-pack%2Fplugins%2Ffleet%2Fserver%2Fsaved_objects%2Findex.ts#L329-L339
</details>

<details><summary>Additional property on existing <code>epm-packages</code> Saved Object</summary>

https://github.com/elastic/kibana/blob/c4f27ab25715a225c710727a37f5f105364e2f72/x-pack/plugins/fleet/server/saved_objects/index.ts#L306-L312

 I don't think the saved object changes are strictly required. It can be removed without changing much about how things work

- Pros: 
      - Preserves accurate record of the assets added at installation time. Separates what assets are currently available for package-version from what was installed. They _should_ be the same, but things happen.
      - Avoids a query to get the installed assets before operating on them
- Cons:
      - size/noise? Could be tens or hundreds of ids
      - migration?
</details>

### More details

**When are saved objects added?**
During installation, after all other actions have succeeded, just before marking the save object as installed, we commit all the files from the package to ES

https://github.com/elastic/kibana/blob/37f7b6ded747edb5cc487661b801c5e1c0a102a7/x-pack%2Fplugins%2Ffleet%2Fserver%2Fservices%2Fepm%2Fpackages%2F_install_package.ts#L193-L198

**When are documents removed from the index?**

In the `removeInstallation` function which is called in response to a `DELETE /api/fleet/epm/packages/pkgkey`

https://github.com/elastic/kibana/blob/37f7b6ded747edb5cc487661b801c5e1c0a102a7/x-pack%2Fplugins%2Ffleet%2Fserver%2Fservices%2Fepm%2Fpackages%2Fremove.ts#L72

or a failed package (re-)installation

https://github.com/elastic/kibana/blob/bf068739acce044ac27902253e8fc31df621f081/x-pack%2Fplugins%2Ffleet%2Fserver%2Fservices%2Fepm%2Fpackages%2Finstall.ts#L145




**How are we using these assets?**
We're not, currently. Here's an example showing how we could update [`getFileHandler`](https://github.com/elastic/kibana/blob/514b50e4c2d7a3be79d77e73838ff57b6cf1304a/x-pack%2Fplugins%2Ffleet%2Fserver%2Froutes%2Fepm%2Fhandlers.ts#L101) to check for local assets before reaching out to the Registry if we wished. It's not DRY, but it does work

```typescript
const esDocRoot = `http://elastic:changeme@localhost:9200/${PACKAGE_ASSETS_INDEX_NAME}/_doc`;
const escapedDocId = encodeURIComponent(`${pkgName}-${pkgVersion}/${filePath}`);
const esRes = await fetch(`${esDocRoot}/${escapedDocId}`);
const esJson = await esRes.json();
if (esJson.found) {
  const asset: PackageAsset = esJson._source;
  const body = asset.data_utf8 || Buffer.from(asset.data_base64, 'base64');
  return response.ok({
    body,
    headers: {
      'content-type': asset.media_type,
      // should add our own `cache-control` header here
      // kibana default is prevents caching: `private, no-cache, no-store, must-revalidate`
      // #83631
    },
  });
}
```

### Checklist
_updated tests to include new saved object output, no tests added yet_
- [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Fleet Team label for Observability Data Collection Fleet team v7.11.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants