-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[ResponseOps][Reporting] Send email with report attachment for scheduled reporting task (merging into feature branch) #220539
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
[ResponseOps][Reporting] Send email with report attachment for scheduled reporting task (merging into feature branch) #220539
Conversation
…:ersin-erdal/kibana into 216308-support-rrule-for-task-scheduling
… src/core/server/integration_tests/ci_checks'
…:ersin-erdal/kibana into 216308-support-rrule-for-task-scheduling
…:ersin-erdal/kibana into 216308-support-rrule-for-task-scheduling
… src/core/server/integration_tests/ci_checks'
…:ersin-erdal/kibana into 216308-support-rrule-for-task-scheduling
afharo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I tested sending the sample dashboard which is ~4Mb with nodemailer and it worked 🎉 .
Left a few more comments and I wasn't able to receive a bcc only email using the exchange API. the spam filter caught it
I see this warning in the logs:
[2025-06-06T13:15:13.486-04:00][WARN ][plugins.actions] Calling "execute" in UnsecuredActionsClient without any relatedSavedObjects data. Consider including this for traceability.
I think we should be able to include the scheduled report SO information
| throw new Error('Reporting notification service has not been initialized.'); | ||
| } | ||
|
|
||
| if (reportSO && reportSO.attributes.notification && reportSO.attributes.notification.email) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess if the reportSO is undefined for some reason, we could try to load it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in this commit, 894e62f
...atform/plugins/private/reporting/server/services/notifications/email_notification_service.ts
Outdated
Show resolved
Hide resolved
| subject, | ||
| html: messageHTML, | ||
| text: message, | ||
| ...(attachments && { attachments }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be ...(attachments.length > 0 && { attachments }),? i don't know if it matters if we're passing an empty array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in this commit, 894e62f
x-pack/platform/plugins/shared/stack_connectors/server/connector_types/email/send_email.ts
Show resolved
Hide resolved
x-pack/platform/plugins/private/reporting/server/lib/tasks/run_scheduled_report.ts
Outdated
Show resolved
Hide resolved
...latform/plugins/shared/stack_connectors/server/connector_types/email/send_email_graph_api.ts
Show resolved
Hide resolved
…_scheduled_report.ts Co-authored-by: Ying Mao <[email protected]>
…alexi/kibana into scheduled-reports-email-attachments
Resolved in this commit, 894e62f |
...atform/plugins/private/reporting/server/services/notifications/email_notification_service.ts
Outdated
Show resolved
Hide resolved
…heduled-reports-email-attachments
x-pack/platform/plugins/private/reporting/server/lib/tasks/run_scheduled_report.ts
Show resolved
Hide resolved
ymao1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| } | ||
| } catch (error) { | ||
| const message = `Error sending notification for scheduled report: ${error.message}`; | ||
| await this.saveExecutionWarning( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does using await here work with the .catch? I think we don't need this await.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in this commit, f3eb0fd
| expect(reportStore.setReportWarning).toHaveBeenCalledWith(savedReport, { | ||
| output: { content_type: 'application/pdf', size: 2097152 }, | ||
| warning: 'Error sending notification for scheduled report: This is a test error!', | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a test for if saveExecutionWarning throws an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and a test that nothing is called if notifications are undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in this commit, f3eb0fd
| const size = Buffer.byteLength(attachment.content); | ||
| if (size < smallAttachmentLimit) { | ||
| // If attachment is smaller than the limit, add the attachment to the draft email | ||
| logger.debug('[MS Exchange] attachment is smaller than 2Mb, attaching to draft'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you could do logger = logger.get('ms-exchange') to add another context to the logger and then it would log as plugins.actions.email.ms-exchange and you woudn't need to add your own [MS Exchange]. logger tags might be useful too...we've been using those more in alerting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in this commit, f3eb0fd
…alexi/kibana into scheduled-reports-email-attachments
…alexi/kibana into scheduled-reports-email-attachments
💛 Build succeeded, but was flaky
Failed CI StepsTest FailuresMetrics [docs]
History
|
Resolves #216313 ## Summary This is a feature branch that contains the following commits. Each individual linked PR contains a summary and verification instructions. * Schedule API - #219771 * Scheduled report task runner - #219770 * List and disable API - #220922 * Audit logging - #221846 * Send scheduled report emails - #220539 * Commit to check license - f5f9d9d * Update to list API response format - #224262 --------- Co-authored-by: Ersin Erdal <[email protected]> Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Ersin Erdal <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Alexi Doak <[email protected]>
Resolves elastic#216313 ## Summary This is a feature branch that contains the following commits. Each individual linked PR contains a summary and verification instructions. * Schedule API - elastic#219771 * Scheduled report task runner - elastic#219770 * List and disable API - elastic#220922 * Audit logging - elastic#221846 * Send scheduled report emails - elastic#220539 * Commit to check license - elastic@f5f9d9d * Update to list API response format - elastic#224262 --------- Co-authored-by: Ersin Erdal <[email protected]> Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Ersin Erdal <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Alexi Doak <[email protected]> (cherry picked from commit a409627) # Conflicts: # src/platform/packages/private/kbn-reporting/common/routes.ts # x-pack/platform/plugins/private/canvas/server/feature.test.ts # x-pack/platform/plugins/private/reporting/server/core.ts # x-pack/platform/plugins/private/reporting/server/features.ts # x-pack/platform/plugins/shared/features/server/__snapshots__/oss_features.test.ts.snap # x-pack/platform/test/api_integration/apis/features/features/features.ts # x-pack/test_serverless/api_integration/test_suites/chat/platform_security/authorization.ts # x-pack/test_serverless/api_integration/test_suites/observability/platform_security/authorization.ts # x-pack/test_serverless/api_integration/test_suites/search/platform_security/authorization.ts # x-pack/test_serverless/api_integration/test_suites/security/platform_security/authorization.ts
# Backport This will backport the following commits from `main` to `8.19`: - [[Response Ops][Reporting] Scheduled Reports (#221028)](#221028) <!--- Backport version: 10.0.1 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Ying Mao","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-06-19T13:20:18Z","message":"[Response Ops][Reporting] Scheduled Reports (#221028)\n\nResolves https://github.com/elastic/kibana/issues/216313\n\n## Summary\n\nThis is a feature branch that contains the following commits. Each\nindividual linked PR contains a summary and verification instructions.\n\n* Schedule API - https://github.com/elastic/kibana/pull/219771\n* Scheduled report task runner -\nhttps://github.com//pull/219770\n* List and disable API - https://github.com/elastic/kibana/pull/220922\n* Audit logging - https://github.com/elastic/kibana/pull/221846\n* Send scheduled report emails -\nhttps://github.com//pull/220539\n* Commit to check license -\nhttps://github.com//pull/221028/commits/f5f9d9daedcd18447b6a02335a53631a44413788\n* Update to list API response format -\nhttps://github.com//pull/224262\n\n---------\n\nCo-authored-by: Ersin Erdal <[email protected]>\nCo-authored-by: kibanamachine <[email protected]>\nCo-authored-by: Ersin Erdal <[email protected]>\nCo-authored-by: Elastic Machine <[email protected]>\nCo-authored-by: Alexi Doak <[email protected]>","sha":"a409627765dfaf3d588c35a0d510b8d1857cd266","branchLabelMapping":{"^v9.1.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:ResponseOps","release_note:feature","ci:cloud-deploy","ci:cloud-persist-deployment","Feature:Reporting:Framework","backport:version","v9.1.0","v8.19.0"],"title":"[Response Ops][Reporting] Scheduled Reports","number":221028,"url":"https://github.com/elastic/kibana/pull/221028","mergeCommit":{"message":"[Response Ops][Reporting] Scheduled Reports (#221028)\n\nResolves https://github.com/elastic/kibana/issues/216313\n\n## Summary\n\nThis is a feature branch that contains the following commits. Each\nindividual linked PR contains a summary and verification instructions.\n\n* Schedule API - https://github.com/elastic/kibana/pull/219771\n* Scheduled report task runner -\nhttps://github.com//pull/219770\n* List and disable API - https://github.com/elastic/kibana/pull/220922\n* Audit logging - https://github.com/elastic/kibana/pull/221846\n* Send scheduled report emails -\nhttps://github.com//pull/220539\n* Commit to check license -\nhttps://github.com//pull/221028/commits/f5f9d9daedcd18447b6a02335a53631a44413788\n* Update to list API response format -\nhttps://github.com//pull/224262\n\n---------\n\nCo-authored-by: Ersin Erdal <[email protected]>\nCo-authored-by: kibanamachine <[email protected]>\nCo-authored-by: Ersin Erdal <[email protected]>\nCo-authored-by: Elastic Machine <[email protected]>\nCo-authored-by: Alexi Doak <[email protected]>","sha":"a409627765dfaf3d588c35a0d510b8d1857cd266"}},"sourceBranch":"main","suggestedTargetBranches":["8.19"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/221028","number":221028,"mergeCommit":{"message":"[Response Ops][Reporting] Scheduled Reports (#221028)\n\nResolves https://github.com/elastic/kibana/issues/216313\n\n## Summary\n\nThis is a feature branch that contains the following commits. Each\nindividual linked PR contains a summary and verification instructions.\n\n* Schedule API - https://github.com/elastic/kibana/pull/219771\n* Scheduled report task runner -\nhttps://github.com//pull/219770\n* List and disable API - https://github.com/elastic/kibana/pull/220922\n* Audit logging - https://github.com/elastic/kibana/pull/221846\n* Send scheduled report emails -\nhttps://github.com//pull/220539\n* Commit to check license -\nhttps://github.com//pull/221028/commits/f5f9d9daedcd18447b6a02335a53631a44413788\n* Update to list API response format -\nhttps://github.com//pull/224262\n\n---------\n\nCo-authored-by: Ersin Erdal <[email protected]>\nCo-authored-by: kibanamachine <[email protected]>\nCo-authored-by: Ersin Erdal <[email protected]>\nCo-authored-by: Elastic Machine <[email protected]>\nCo-authored-by: Alexi Doak <[email protected]>","sha":"a409627765dfaf3d588c35a0d510b8d1857cd266"}},{"branch":"8.19","label":"v8.19.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> --------- Co-authored-by: kibanamachine <[email protected]>
Resolves #216316
Note
This PR will be merged into a feature branch
Summary
This PR updates the schedule reporting to flow to notify users once the report is generated successfully and adds support for attachments in the email connector. There is a 10 MB limit for schedule report attachments, if the attachment is larger than 10MB it will throw an error and save it in the scheduled report.
The flow for sending attachments using Nodemailer is simple, we just append them to the request.
For Exchange, the flow is more complicated:
Checklist
To verify
notifications.connectors.default.email. We need to test this for Exchange and Nodemailer. Slack me for the config and secrets to test Exchange. For nodemailer you can use maildev.