[Reporting] Use spaceId from request in export generation #76998

tsullivan · 2020-09-08T22:39:57Z

Summary

Release note: Fixed the reporting exports to use the correct Space for advanced settings.

Explanation:
Reporting builds a fakeRequest object to use as a source that provides UI Settings. If the request does not have the space ID baked in, the settings will always come from the default space.

Right now, none of the fake request objects have a space ID baked in, so the default space settings are always used. This affects:

CSV: csv:separator and csv:quoteValues are space-specific settings
PDF: xpackReporting:customPdfLogo is space-specific

This PR captures the spaceId from the request at job creation time. It plays into the fakeRequest objects at job execution time using:

core.http.basePath.set(fakeRequest, `/s/${spaceId}`)

It replaces code from a very old PR that incorporated the basePath of the request URL to determine the space ID. It then would incorporate the spaceId into fake requests by adding a getBasePath method, which was expected to return the space ID along with the base path. In the new platform, there is no getBasePath method of KibanaRequest, which makes the basePath handling on the server side useless. This PR removes the basePath field from job params since it isn't useful to receive it.

Fix #68076
Fix #70175
Fix: PDF Logo is always taken from default space

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

tsullivan · 2020-09-09T21:57:10Z

x-pack/plugins/reporting/server/export_types/common/get_custom_logo.ts

- // This is used by the spaces SavedObjectClientWrapper to determine the existing space.
- // We use the basePath from the saved job, which we'll have post spaces being implemented;
- // or we use the server base path, which uses the default space
- getBasePath: () => job.basePath || serverBasePath,


This is the no-longer-working code for supporting spaces in our UI Settings clients.

x-pack/plugins/reporting/server/export_types/csv/execute_job.ts

x-pack/plugins/reporting/server/export_types/printable_pdf/execute_job/index.ts

tsullivan · 2020-09-10T22:40:26Z

x-pack/plugins/reporting/server/export_types/printable_pdf/execute_job/index.ts

- mergeMap((conditionalHeaders) =>
- getCustomLogo({ reporting, config, job, conditionalHeaders })
- ),
+ mergeMap((conditionalHeaders) => getCustomLogo(reporting, conditionalHeaders, job.spaceId)),


I refactored getCustomLogo to receive more specific parameters. The other common helper functions here could probably use the same thing

elasticmachine · 2020-09-10T22:41:42Z

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

…/get_custom_logo.ts Co-authored-by: Joel Griffith <[email protected]>

tsullivan · 2020-09-18T16:51:45Z

x-pack/plugins/reporting/server/core.ts

+ if (spacesService) {
+ if (spaceId && spaceId !== DEFAULT_SPACE_ID) {
+ this.logger.info(`Generating request for space: ` + spaceId);
+ this.getPluginSetupDeps().basePath.set(fakeRequest, `/s/${spaceId}`);


cc @jportner Thanks for your help in figuring this part out! Please take a look at how I am using your advice and let me know if you have any thoughts about it.

As discussed offline -- looks great!

x-pack/test/reporting_api_integration/reporting_and_security/spaces.ts

tsullivan · 2020-09-18T18:23:58Z

@elasticmachine merge upstream

) * [Reporting] Use spaceId from request in export generation * remove todo that has been done * whitespace * use post params api in test * add logging to core * Update x-pack/plugins/reporting/server/export_types/printable_pdf/lib/get_custom_logo.ts Co-authored-by: Joel Griffith <[email protected]> * more logging * fix interdependence and remove Promise.all * getAbsoluteUrl have only 1 way to provide basePath * --wip-- [skip ci] * log apipath * deleteAllReports at the end * tests pass locally * set config in the tests * re-add skips of flaky tests * test using csv:quoteValues Co-authored-by: Joel Griffith <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

kibanamachine · 2020-09-18T20:19:26Z

💛 Build succeeded, but was flaky

continuous-integration/kibana-ci/pull-request
Commit: 9a33e8b
Flaky suites:
- xpack-kibana-ciGroup1

Test Failures

X-Pack APM API integration tests (basic).x-pack/test/apm_api_integration/basic/tests/metrics_charts/metrics_charts·ts.APM specs (basic) Metrics when data is loaded for opbeans-node returns metrics data CPU usage has correct series overall values

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 2 times on tracked branches: https:/elastic/kibana/issues/77870

[00:00:00]       │
[00:00:00]         └-: APM specs (basic)
[00:00:00]           └-> "before all" hook
[00:01:42]           └-: Metrics
[00:01:42]             └-> "before all" hook
[00:01:42]             └-: when data is loaded
[00:01:42]               └-> "before all" hook
[00:01:42]               └-> "before all" hook
[00:01:42]                 │ info [metrics_8.0.0] Loading "mappings.json"
[00:01:42]                 │ info [metrics_8.0.0] Loading "data.json.gz"
[00:01:42]                 │ info [o.e.c.m.MetadataCreateIndexService] [kibana-ci-immutable-centos-tests-xxl-1600453518999560881] [apm-8.0.0-metric-000001] creating index, cause [api], templates [], shards [1]/[0]
[00:01:42]                 │ info [o.e.c.r.a.AllocationService] [kibana-ci-immutable-centos-tests-xxl-1600453518999560881] current.health="GREEN" message="Cluster health status changed from [YELLOW] to [GREEN] (reason: [shards started [[apm-8.0.0-metric-000001][0]]])." previous.health="YELLOW" reason="shards started [[apm-8.0.0-metric-000001][0]]"
[00:01:42]                 │ info [metrics_8.0.0] Created index "apm-8.0.0-metric-000001"
[00:01:42]                 │ debg [metrics_8.0.0] "apm-8.0.0-metric-000001" settings {"index":{"codec":"best_compression","lifecycle":{"name":"apm-rollover-30-days","rollover_alias":"apm-8.0.0-metric"},"mapping":{"total_fields":{"limit":"2000"}},"max_docvalue_fields_search":"200","number_of_replicas":"0","number_of_shards":"1","priority":"100","refresh_interval":"1ms"}}
[00:01:42]                 │ info [metrics_8.0.0] Indexed 2323 docs into "apm-8.0.0-metric-000001"
[00:01:42]               └-: for opbeans-node
[00:01:42]                 └-> "before all" hook
[00:01:42]                 └-: returns metrics data
[00:01:42]                   └-> "before all" hook
[00:01:42]                   └-> "before all" hook
[00:01:43]                   └-> contains CPU usage and System memory usage chart data
[00:01:43]                     └-> "before each" hook: global before each
[00:01:43]                     └-> "before each" hook
[00:01:43]                     └- ✓ pass  (1ms) "APM specs (basic) Metrics when data is loaded for opbeans-node returns metrics data contains CPU usage and System memory usage chart data"
[00:01:43]                   └-> "after each" hook
[00:01:43]                   └-: CPU usage
[00:01:43]                     └-> "before all" hook
[00:01:43]                     └-> "before all" hook
[00:01:43]                     └-> has correct series
[00:01:43]                       └-> "before each" hook: global before each
[00:01:43]                       └-> "before each" hook
[00:01:43]                       └- ✓ pass  (0ms) "APM specs (basic) Metrics when data is loaded for opbeans-node returns metrics data CPU usage has correct series"
[00:01:43]                     └-> "after each" hook
[00:01:43]                     └-> has correct series overall values
[00:01:43]                       └-> "before each" hook: global before each
[00:01:43]                       └-> "before each" hook
[00:01:43]                       └- ✖ fail: APM specs (basic) Metrics when data is loaded for opbeans-node returns metrics data CPU usage has correct series overall values
[00:01:43]                       │       Error: expect(received).toMatchInlineSnapshot(snapshot)
[00:01:43]                       │ 
[00:01:43]                       │ Snapshot name: `when data is loaded for opbeans-node returns metrics data CPU usage has correct series overall values 1`
[00:01:43]                       │ 
[00:01:43]                       │ - Snapshot  - 1
[00:01:43]                       │ + Received  + 1
[00:01:43]                       │ 
[00:01:43]                       │   Array [
[00:01:43]                       │     0.714,
[00:01:43]                       │ -   0.38770000000000004,
[00:01:43]                       │ +   0.3877,
[00:01:43]                       │     0.75,
[00:01:43]                       │     0.2543,
[00:01:43]                       │   ]
[00:01:43]                       │       + expected - actual
[00:01:43]                       │ 
[00:01:43]                       │       -false
[00:01:43]                       │       +true
[00:01:43]                       │       
[00:01:43]                       │       at Assertion.assert (/dev/shm/workspace/parallel/15/kibana/packages/kbn-expect/expect.js:100:11)
[00:01:43]                       │       at Assertion.toMatchInline [as eql] (/dev/shm/workspace/parallel/15/kibana/packages/kbn-expect/expect.js:244:8)
[00:01:43]                       │       at Context.apply (test/apm_api_integration/basic/tests/metrics_charts/metrics_charts.ts:69:16)
[00:01:43]                       │       at Object.apply (/dev/shm/workspace/parallel/15/kibana/packages/kbn-test/src/functional_test_runner/lib/mocha/wrap_function.js:84:30)
[00:01:43]                       │ 
[00:01:43]                       │

Stack Trace

{ Error: expect(received).toMatchInlineSnapshot(snapshot)

Snapshot name: `when data is loaded for opbeans-node returns metrics data CPU usage has correct series overall values 1`

- Snapshot  - 1
+ Received  + 1

  Array [
    0.714,
-   0.38770000000000004,
+   0.3877,
    0.75,
    0.2543,
  ]
    at Assertion.assert (/dev/shm/workspace/parallel/15/kibana/packages/kbn-expect/expect.js:100:11)
    at Assertion.toMatchInline [as eql] (/dev/shm/workspace/parallel/15/kibana/packages/kbn-expect/expect.js:244:8)
    at Context.apply (test/apm_api_integration/basic/tests/metrics_charts/metrics_charts.ts:69:16)
    at Object.apply (/dev/shm/workspace/parallel/15/kibana/packages/kbn-test/src/functional_test_runner/lib/mocha/wrap_function.js:84:30) actual: 'false', expected: 'true', showDiff: true }

Build metrics

distributable file count

id	value	diff	baseline
default	45938	-1	45939

History

💚 Build #75672 succeeded 068219a
💚 Build #75642 succeeded 98e3db0
💛 Build #75441 was flaky 1b0f545
💔 Build #75417 failed db65847
💔 Build #75411 failed 77e4ad0

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

…77952) * [Reporting] Use spaceId from request in export generation * remove todo that has been done * whitespace * use post params api in test * add logging to core * Update x-pack/plugins/reporting/server/export_types/printable_pdf/lib/get_custom_logo.ts Co-authored-by: Joel Griffith <[email protected]> * more logging * fix interdependence and remove Promise.all * getAbsoluteUrl have only 1 way to provide basePath * --wip-- [skip ci] * log apipath * deleteAllReports at the end * tests pass locally * set config in the tests * re-add skips of flaky tests * test using csv:quoteValues Co-authored-by: Joel Griffith <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Joel Griffith <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

tsullivan force-pushed the fix/68076 branch 2 times, most recently from dd19128 to 9ef809a Compare September 9, 2020 21:52

tsullivan commented Sep 9, 2020

View reviewed changes

x-pack/plugins/reporting/server/export_types/csv/execute_job.ts Show resolved Hide resolved

tsullivan force-pushed the fix/68076 branch 3 times, most recently from a16c44c to baafb3f Compare September 9, 2020 22:34

tsullivan changed the title ~~Fix/68076~~ [Reporting] Use spaceId from request in export generation Sep 9, 2020

tsullivan force-pushed the fix/68076 branch 12 times, most recently from 1acd05e to 1953026 Compare September 10, 2020 22:33

tsullivan commented Sep 10, 2020

View reviewed changes

x-pack/plugins/reporting/server/export_types/printable_pdf/execute_job/index.ts Show resolved Hide resolved

tsullivan force-pushed the fix/68076 branch from 1953026 to d041bc4 Compare September 10, 2020 22:39

tsullivan commented Sep 10, 2020

View reviewed changes

tsullivan marked this pull request as ready for review September 10, 2020 22:41

tsullivan added release_note:fix Team:Reporting Services v8.0.0 labels Sep 10, 2020

tsullivan added the v7.10.0 label Sep 10, 2020

tsullivan requested a review from joelgriffith September 10, 2020 23:25

tsullivan and others added 13 commits September 17, 2020 15:45

add logging to core

3ac614a

Update x-pack/plugins/reporting/server/export_types/printable_pdf/lib…

f5efc5b

…/get_custom_logo.ts Co-authored-by: Joel Griffith <[email protected]>

more logging

77e4ad0

fix interdependence and remove Promise.all

8e31474

getAbsoluteUrl have only 1 way to provide basePath

db65847

Merge branch 'master' of github.com:elastic/kibana into fix/68076

1bab508

--wip-- [skip ci]

545c277

log apipath

7300e9b

deleteAllReports at the end

3b2b45c

tests pass locally

c28c048

set config in the tests

1b0f545

Merge branch 'master' into fix/68076

c7b1da7

re-add skips of flaky tests

98e3db0

tsullivan commented Sep 18, 2020

View reviewed changes

joelgriffith reviewed Sep 18, 2020

View reviewed changes

x-pack/test/reporting_api_integration/reporting_and_security/spaces.ts Show resolved Hide resolved

joelgriffith approved these changes Sep 18, 2020

View reviewed changes

test using csv:quoteValues

a5c2171

elasticmachine and others added 2 commits September 18, 2020 14:24

Merge branch 'master' into fix/68076

9a33e8b

Merge branch 'fix/68076' of github.com:tsullivan/kibana into fix/68076

068219a

tsullivan merged commit 6908fe7 into elastic:master Sep 18, 2020

tsullivan deleted the fix/68076 branch September 18, 2020 20:08

tsullivan mentioned this pull request Sep 18, 2020

[7.x] [Reporting] Use spaceId from request in export generation (#76998) #77952

Merged

tsullivan mentioned this pull request Oct 1, 2020

[Reporting] Improve logging about re-creating requests #79209

Closed

1 task

Dosant mentioned this pull request Dec 4, 2020

CSV report is using timezone from default Space instead of the current Space #70175

Closed

jportner mentioned this pull request Mar 15, 2021

Fix invalid Reporting integration test data #94616

Merged

sophiec20 added the (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead label Aug 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reporting] Use spaceId from request in export generation #76998

[Reporting] Use spaceId from request in export generation #76998

tsullivan commented Sep 8, 2020 •

edited

Loading

tsullivan Sep 9, 2020 •

edited

Loading

tsullivan Sep 10, 2020

elasticmachine commented Sep 10, 2020

tsullivan Sep 18, 2020

jportner Sep 18, 2020

tsullivan commented Sep 18, 2020

kibanamachine commented Sep 18, 2020

Standard Out

Stack Trace

[Reporting] Use spaceId from request in export generation #76998

[Reporting] Use spaceId from request in export generation #76998

Conversation

tsullivan commented Sep 8, 2020 • edited Loading

Summary

Checklist

For maintainers

tsullivan Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

tsullivan Sep 10, 2020

Choose a reason for hiding this comment

elasticmachine commented Sep 10, 2020

tsullivan Sep 18, 2020

Choose a reason for hiding this comment

jportner Sep 18, 2020

Choose a reason for hiding this comment

tsullivan commented Sep 18, 2020

kibanamachine commented Sep 18, 2020

💛 Build succeeded, but was flaky

Test Failures

Standard Out

Stack Trace

Build metrics

distributable file count

History

tsullivan commented Sep 8, 2020 •

edited

Loading

tsullivan Sep 9, 2020 •

edited

Loading