Skip to content

Conversation

@shahargl
Copy link
Contributor

Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Documentation was added for features that require explanation or tutorials
  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
  • This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
  • Flaky Test Runner was used on any tests changed
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines
  • Review the backport guidelines and apply applicable backport:* labels.

Identify risks

Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.

@shahargl shahargl requested a review from a team as a code owner July 30, 2025 11:00
shahargl and others added 10 commits July 30, 2025 14:19
- Created full WorkflowEventLoggerService with ES search capabilities
- Restored getExecutionLogs and getStepLogs with actual ES queries
- Fixed workflows management service to use proper logger service
- Maintained console logging functionality while adding back log search
- Remove imports from workflows_management to workflows_execution_engine
- Implement direct ES log search in management service
- Keep simple console logging without complex service layer
- Clean plugin separation with no circular dependencies
- Fix data structure access: log['@timestamp'] instead of log.source['@timestamp']
- Add defensive filtering to remove undefined/null log entries
- Improve error handling in both service and API layers
- Handle ES responses that may have missing _source data
@shahargl shahargl requested a review from Copilot July 30, 2025 13:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the workflow logging system to introduce console output support and simplify the logging architecture. The changes migrate from a complex WorkflowEventLoggerService to a simpler SimpleWorkflowLogger with optional console logging, while adding new methods for searching workflow execution logs directly.

  • Replaces complex WorkflowEventLoggerService with a simplified SimpleWorkflowLogger that supports console output
  • Adds direct Elasticsearch query methods for retrieving workflow, execution, and step logs
  • Introduces console logging configuration options in both workflow management and execution engine plugins

Reviewed Changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
workflows_management_service.ts Replaces WorkflowEventLoggerService with SimpleWorkflowLogger and adds direct ES log search methods
workflows_management_api.ts Updates log transformation to handle new log structure and field mappings
workflow_logger.ts New simple logger implementation with console logging support
workflow_event_logger_service.ts Removes complex logging service (deleted file)
config.ts Adds console logging configuration option for both plugins
plugin.ts Integrates console logging configuration into plugin initialization
workflow_event_logger_service.ts (execution engine) Adds comprehensive logging service for execution engine with console support
workflow_context_manager.ts Re-enables workflow logging functionality with console output support
Comments suppressed due to low confidence (2)

src/platform/plugins/shared/workflows_management/server/workflows_management/workflows_management_service.ts:57

  • [nitpick] The parameter name 'enableConsoleLogging' is inconsistent with the config property name 'logging.console'. Consider renaming to 'consoleLogging' or 'enableConsole' to match the configuration structure.
    enableConsoleLogging: boolean = false

src/platform/plugins/shared/workflows_management/server/workflows_management/workflows_management_service.ts:80

  • The property 'workflowEventLoggerService' name is misleading since it now holds a SimpleWorkflowLogger instance, not a service. Consider renaming to 'workflowLogger' or 'simpleWorkflowLogger'.
    this.workflowEventLoggerService = new SimpleWorkflowLogger(this.logger, enableConsoleLogging);

typeof response.hits.total === 'number'
? response.hits.total
: response.hits.total?.value || 0,
logs: response.hits.hits.map((hit: any) => hit._source),
Copy link

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using 'any' type for Elasticsearch hits loses type safety. Consider defining a proper interface for the hit structure or at least the _source field to improve type safety.

Suggested change
logs: response.hits.hits.map((hit: any) => hit._source),
logs: response.hits.hits.map((hit) => (hit._source as WorkflowLogSource)),

Copilot uses AI. Check for mistakes.
@shahargl shahargl enabled auto-merge (squash) July 31, 2025 10:48
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

@shahargl shahargl added the release_note:skip Skip the PR/issue when compiling release notes label Jul 31, 2025
@shahargl shahargl merged commit 7ad9493 into elastic:main Jul 31, 2025
11 of 12 checks passed
@kibanamachine kibanamachine added v9.2.0 backport:skip This PR does not require backporting labels Jul 31, 2025
@talboren talboren added the Team:One Workflow Team label for One Workflow (Workflow automation) label Jul 31, 2025
delanni pushed a commit to delanni/kibana that referenced this pull request Aug 5, 2025
## Summary

Summarize your PR. If it involves visual changes include a screenshot or
gif.


### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [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
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
- [ ] Review the [backport
guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing)
and apply applicable `backport:*` labels.

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
@wildemat wildemat mentioned this pull request Aug 7, 2025
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:One Workflow Team label for One Workflow (Workflow automation) v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants