[Reporting] Track retryAt and startedAt times on the task instance #174409

tsullivan · 2024-01-06T00:16:38Z

Summary

Initial part of #131852

This PR moves towards auto-calculating the maximum timeouts calculated based on the timing context provided by the running task instance.

Other changes

Added an optional logger parameter to the getScreenshots function. When a logger is provided, the logs created by the screenshot plugin will have the contextual tags added by the calling code.
- Before
- After
Fixed an unreported bug where browser timezone was not utilized in PNG reports.

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

tsullivan · 2024-01-10T17:09:00Z

/ci

tsullivan · 2024-01-10T22:40:30Z

packages/kbn-reporting/export_types/png/png_v2.ts

@@ -122,9 +119,12 @@ export class PngExportType extends ExportType<JobParamsPNGV2, TaskPayloadPNGV2>
        return this.startDeps
          .screenshotting!.getScreenshots({
            format: 'png',
+            browserTimezone: payload.browserTimezone,


Oops! browserTimezone was not being passed before. That could have caused PNG reports showing date values formatted in the timezone of the Kibana server rather than the end user client. It doesn't seem to be a previosly known issue.

…tsullivan/kibana into reporting-taskinstance-retryat-tracking

tsullivan · 2024-01-11T22:31:20Z

packages/kbn-reporting/server/types.ts

@@ -38,6 +43,7 @@ export type CreateJobFn<JobParamsType = BaseParams, JobPayloadType = BasePayload
 export type RunTaskFn<TaskPayloadType = BasePayload> = (
  jobId: string,
  payload: TaskPayloadType,
+  taskInstanceFields: TaskInstanceFields,


To make the task lifecycle timing information available to the run task functions, I just added this new parameter to for all Run Task functions.

tsullivan · 2024-01-11T22:32:55Z

packages/kbn-reporting/export_types/csv/csv_searchsource_immediate.ts

@@ -108,9 +108,11 @@ export class CsvSearchSourceImmediateExportType extends ExportType<
    };
    const cancellationToken = new CancellationToken();
    const csvConfig = this.config.csv;
+    const taskInstanceFields = { startedAt: null, retryAt: null };


Here is another example of why the "immediate" CSV export type doesn't fit well into the reporting framework: getting the export available to download immediately doesn't go through Task Manager.

Related: #164104

eokoneyo

LGTM, I left couple of questions that's mostly nits

eokoneyo · 2024-01-16T12:40:35Z

x-pack/plugins/reporting/server/lib/tasks/execute_report.ts

@@ -430,7 +431,7 @@ export class ExecuteReportTask implements ReportingTask {
            eventLog.logExecutionStart();

            const output = await Promise.race<TaskRunResult>([
-              this._performJob(task, cancellationToken, stream),
+              this._performJob(task, taskInstance, cancellationToken, stream),


should we pass just the properties we defined for the TaskInstanceFields type instead of the whole taskInstanceField object received from the task manager?

Thanks! a4b5189

eokoneyo · 2024-01-16T12:47:15Z

x-pack/plugins/screenshotting/common/types.ts

+/**
+ * Timestamp metrics about a running screenshot task
+ * which determine the maximum timeouts possible
+ */
+export interface TaskInstanceFields {
+  /**
+   * The date and time that this task started execution. This is used to determine
+   * the "real" runAt that ended up running the task. This value is only set
+   * when status is set to "running".
+   */
+  startedAt: Date | null;
+
+  /**
+   * The date and time that this task should re-execute if stuck in "running" / timeout
+   * status. This value is only set when status is set to "running".
+   */
+  retryAt: Date | null;
+}


nit: I think we can create the same type definition we have here https://github.com/elastic/kibana/pull/174409/files#diff-f4342d4c59ca76e69910849e6c0b35157d2c4cb0da0d7bc91fd632720ab2c0e0 that selects from ConcreteTaskInstance

👍🏻 b377d00 4ac9663

…fix'

… pipeline

…tsullivan/kibana into reporting-taskinstance-retryat-tracking

kibana-ci · 2024-01-16T21:40:42Z

💚 Build Succeeded

Buildkite Build
Commit: 95ca8d3

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/generate-csv`	10	11	+1
`@kbn/reporting-export-types-csv`	50	52	+2
`@kbn/reporting-export-types-pdf`	31	33	+2
`@kbn/reporting-export-types-png`	15	16	+1
`@kbn/reporting-server`	77	79	+2
total			+8

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/reporting-common`	7	8	+1

Unknown metric groups

API count

id	before	after	diff
`@kbn/generate-csv`	10	11	+1
`@kbn/reporting-export-types-csv`	50	52	+2
`@kbn/reporting-export-types-pdf`	32	34	+2
`@kbn/reporting-export-types-png`	16	17	+1
`@kbn/reporting-server`	78	80	+2
total			+8

History

💚 Build #187086 succeeded 3f520a8
💚 Build #187070 succeeded 18cb825
💔 Build #187023 failed af7b739
💔 Build #186909 failed f1c1b2e

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

## Summary Initial part of elastic#131852 This PR moves towards auto-calculating the maximum timeouts calculated based on the timing context provided by the running task instance. ### Other changes * Added an optional logger parameter to the `getScreenshots` function. When a logger is provided, the logs created by the screenshot plugin will have the contextual tags added by the calling code. * Before <img width="1198" alt="image" src="https://github.com/elastic/kibana/assets/908371/f68a102e-6af2-4863-aedb-52f1e4a099d8"> * After <img width="1200" alt="image" src="https://github.com/elastic/kibana/assets/908371/2dd4c947-ffa6-4cb3-b8a2-22893f49ddb7"> * Fixed an unreported bug where browser timezone was not utilized in PNG reports. ### Checklist Delete any items that are not applicable to this PR. - [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

tsullivan force-pushed the reporting-taskinstance-retryat-tracking branch 2 times, most recently from 22c1bfc to 742e20f Compare January 10, 2024 17:07

tsullivan force-pushed the reporting-taskinstance-retryat-tracking branch 3 times, most recently from ab82ea1 to ce7307f Compare January 10, 2024 20:06

Use timebased-fields task instance in export execution

df0f8ee

tsullivan force-pushed the reporting-taskinstance-retryat-tracking branch from ce7307f to 5773365 Compare January 10, 2024 20:47

Allow custom logger for getScreenshots

9c5ec58

tsullivan force-pushed the reporting-taskinstance-retryat-tracking branch from 5773365 to 9c5ec58 Compare January 10, 2024 21:47

Update unit tests & fix PNG timezone issue

b7c297d

tsullivan commented Jan 10, 2024

View reviewed changes

tsullivan marked this pull request as ready for review January 10, 2024 22:41

tsullivan requested review from a team as code owners January 10, 2024 22:41

Merge branch 'main' into reporting-taskinstance-retryat-tracking

4525063

tsullivan added the release_note:skip Skip the PR/issue when compiling release notes label Jan 10, 2024

elastic deleted a comment from kibana-ci Jan 10, 2024

tsullivan added 3 commits January 10, 2024 15:47

polish

97ed937

Merge branch 'reporting-taskinstance-retryat-tracking' of github.com:…

aef1e34

…tsullivan/kibana into reporting-taskinstance-retryat-tracking

fix the errors of types

f1c1b2e

elastic deleted a comment from kibana-ci Jan 11, 2024

tsullivan added 2 commits January 11, 2024 08:46

Merge branch 'main' into reporting-taskinstance-retryat-tracking

54edc21

fix more typings

af7b739

tsullivan mentioned this pull request Jan 11, 2024

[Reporting] add an "auto" option for the timeout settings #131852

Closed

tsullivan added the backport:skip This commit does not require backporting label Jan 11, 2024

tsullivan added 2 commits January 11, 2024 13:03

Merge branch 'main' into reporting-taskinstance-retryat-tracking

dad3c92

fix tests & final cleanup

18cb825

tsullivan commented Jan 11, 2024

View reviewed changes

Remove tests that test nothing

3f520a8

eokoneyo approved these changes Jan 16, 2024

View reviewed changes

tsullivan and others added 3 commits January 16, 2024 13:09

Merge branch 'main' into reporting-taskinstance-retryat-tracking

70e1e0b

pass just the properties that are defined for taskInstanceFields

a4b5189

consistent type definitions for TaskInstanceFields

b377d00

tsullivan enabled auto-merge (squash) January 16, 2024 20:21

tsullivan changed the title ~~[Reporting] Track retryAt time on the task instance~~ [Reporting] Track retryAt and startedAt times on the task instance Jan 16, 2024

kibanamachine and others added 3 commits January 16, 2024 20:26

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

4ac9663

…fix'

New way to test that the contextual logger is used for screenshotting…

bd79175

… pipeline

Merge branch 'reporting-taskinstance-retryat-tracking' of github.com:…

95ca8d3

…tsullivan/kibana into reporting-taskinstance-retryat-tracking

tsullivan merged commit decec37 into elastic:main Jan 16, 2024
19 checks passed

kibanamachine added the v8.13.0 label Jan 16, 2024

tsullivan deleted the reporting-taskinstance-retryat-tracking branch April 30, 2024 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reporting] Track retryAt and startedAt times on the task instance #174409

[Reporting] Track retryAt and startedAt times on the task instance #174409

tsullivan commented Jan 6, 2024 •

edited

Loading

tsullivan commented Jan 10, 2024

tsullivan Jan 10, 2024

tsullivan Jan 11, 2024

tsullivan Jan 11, 2024

eokoneyo left a comment •

edited

Loading

eokoneyo Jan 16, 2024

tsullivan Jan 16, 2024

eokoneyo Jan 16, 2024

tsullivan Jan 16, 2024 •

edited

Loading

kibana-ci commented Jan 16, 2024

API count

[Reporting] Track retryAt and startedAt times on the task instance #174409

[Reporting] Track retryAt and startedAt times on the task instance #174409

Conversation

tsullivan commented Jan 6, 2024 • edited Loading

Summary

Other changes

Checklist

tsullivan commented Jan 10, 2024

tsullivan Jan 10, 2024

Choose a reason for hiding this comment

tsullivan Jan 11, 2024

Choose a reason for hiding this comment

tsullivan Jan 11, 2024

Choose a reason for hiding this comment

eokoneyo left a comment • edited Loading

Choose a reason for hiding this comment

eokoneyo Jan 16, 2024

Choose a reason for hiding this comment

tsullivan Jan 16, 2024

Choose a reason for hiding this comment

eokoneyo Jan 16, 2024

Choose a reason for hiding this comment

tsullivan Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

kibana-ci commented Jan 16, 2024

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

API count

History

tsullivan commented Jan 6, 2024 •

edited

Loading

eokoneyo left a comment •

edited

Loading

tsullivan Jan 16, 2024 •

edited

Loading