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

[filebeat][httpjson] - Separation of global transform contexts and introduction of parent transform context within chains #33499

Merged
merged 11 commits into from
Nov 8, 2022

Conversation

ShourieG
Copy link
Contributor

@ShourieG ShourieG commented Oct 31, 2022

Type of change

  • Enhancement

What does this PR do?

This PR introduces the following enhancements to httpjson -

  1. It introduces separation of transform contexts from parent (root) requests and chain requests.
  2. It introduces a parent transform context object parentTrCtx as a new attribute of trCtx (main transform context
    object) which is a self referential pointer and this will point to the transform context of the root request when accessed
    from within chain steps.
  3. It introduces the .parent_last_response.* clause, which helps access the last_response of the parent request
    during chaining.

Why is it important?

  1. This change helps improve clarity between chain steps & non-chain steps by clearly maintaining separate instances
    of their respective transform contexts, hence reducing the chances of unexpected errors due to overlapping transform
    contexts.
  2. By introducing the .parent_last_response.* clause & the parentTrCtx pointer, we open up various new use-
    cases to be implemented. For example a request with chaining, where the root request on every page response
    receives an unique id in the body, which is then passed to the respective chain requests to extract the required
    information. The test included simulates this scenario.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
    -[ ] I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc.

Author's Checklist

  • Code structure , syntax & comments
  • Code optimization & quality

Related issues

@ShourieG ShourieG requested a review from a team as a code owner October 31, 2022 14:06
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 31, 2022
@mergify
Copy link
Contributor

mergify bot commented Oct 31, 2022

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @ShourieG? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@elasticmachine
Copy link
Collaborator

elasticmachine commented Oct 31, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-11-08T05:54:30.392+0000

  • Duration: 73 min 38 sec

Test stats 🧪

Test Results
Failed 0
Passed 2315
Skipped 167
Total 2482

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Nov 1, 2022
@mergify
Copy link
Contributor

mergify bot commented Nov 2, 2022

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b httpjson/trCtx upstream/httpjson/trCtx
git merge upstream/main
git push upstream httpjson/trCtx

Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

Minor comment, otherwise LGTM.

x-pack/filebeat/docs/inputs/input-httpjson.asciidoc Outdated Show resolved Hide resolved
x-pack/filebeat/docs/inputs/input-httpjson.asciidoc Outdated Show resolved Hide resolved
x-pack/filebeat/input/httpjson/request.go Show resolved Hide resolved
x-pack/filebeat/input/httpjson/input_test.go Outdated Show resolved Hide resolved
x-pack/filebeat/input/httpjson/input_test.go Outdated Show resolved Hide resolved
x-pack/filebeat/docs/inputs/input-httpjson.asciidoc Outdated Show resolved Hide resolved
@ShourieG
Copy link
Contributor Author

ShourieG commented Nov 4, 2022

@efd6 have made the necessary improvements with updated docs and test cases. Now dot's in expressions and hard coded values will be properly processed.

@ShourieG
Copy link
Contributor Author

ShourieG commented Nov 7, 2022

@efd6 updated the docs and the processExpression method. In the end decided to keep the default behaviour as it made more sense. The docs have been updated accordingly and test cases have been add to support the same.

@ShourieG
Copy link
Contributor Author

ShourieG commented Nov 8, 2022

@efd6 updated PR with the processException tests.

Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

Thanks

@ShourieG ShourieG merged commit 09a9b55 into elastic:main Nov 8, 2022
chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
…troduction of parent transform context within chains (#33499)

* initial commit for transform context separation and introduction of parent object

* removed ioutil from tests

* updated asciidoc changelog

* added support for dot's in values and expressions for replace_with clause, added doc updates and tests

* added linter ignores for errcheck in test scenario

* updated as per pr suggetions

* added processExpression Tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpJson new feature - Introduce a global transform context, accessible from within chain calls
4 participants