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.

Sign up for GitHub

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

Jump to bottom

[elastic/beats] [HttpJson] - Improves request chaining with new 'while' step & client creation per step #32222

Merged
merged 15 commits into from
Jul 14, 2022
Merged

[elastic/beats] [HttpJson] - Improves request chaining with new 'while' step & client creation per step #32222

merged 15 commits into from
Jul 14, 2022

Conversation

ShourieG
Copy link
Contributor

@ShourieG ShourieG commented Jul 6, 2022 •
edited
Loading

Type of change

- Enhancement

What does this PR do?

  1. Introduced new request chain step 'while' to tackle scenarios as reported in this ticket : [Filebeat][httpjson input] Add status check API calls.  #29959

  2. Added support for creation of of new http client per step in the request chain , thereby enabling individual steps to
    take advantage of unique auth's per step if required and access to all request parameters. This also helps lay the foundations
    for solving the the issue : [Filebeat][Inputs][HTTP JSON] Multi-Step/API Requests #31117.
    [Note] : if an auth per step is not defined , the steps shall inherit any auth that is used in the root request call.

  3. Added an improvement which now removes the requirement for request.retry.wait_max field to be specified everywhere
    even when it is not required. Now you can simply specify request.retry.wait_min for the retry config and the wait will occur.
    You can still specify both fields ..when they have different values. Previously only specifying request.retry.wait_min caused
    the wait not not occur.

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 input-httpjson.asciidoc .

Related issues

@ShourieG ShourieG requested a review from a team as a code owner July 6, 2022 11:00
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jul 6, 2022
@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 Jul 6, 2022
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jul 6, 2022 •
edited by jenkins-beats-ci bot
Loading

💚 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-07-13T13:29:49.433+0000

  • Duration: 79 min 20 sec

Test stats 🧪

Test Results
Failed 0
Passed 2170
Skipped 166
Total 2336

💚 Flaky test report

Tests succeeded.

🤖 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!)

@ShourieG ShourieG changed the title [HttpJson] - Improves request chaining with new 'while' step & client creation per step [elastic/beats] [HttpJson] - Improves request chaining with new 'while' step & client creation per step Jul 6, 2022
efd6
efd6 reviewed Jul 7, 2022
View reviewed changes
@ShourieG ShourieG requested review from marc-gr, P1llus and adriansr July 8, 2022 05:50
efd6
efd6 reviewed Jul 11, 2022
View reviewed changes
adriansr
adriansr suggested changes Jul 12, 2022
View reviewed changes
Copy link
Contributor

@adriansr adriansr left a comment

Choose a reason for hiding this comment

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

Added some questions to review, mostly around docs/language. Please have a look.

I see that some of the issues with the docs were already present, I've created #32321 to remember to fix them.

@adriansr adriansr self-requested a review July 13, 2022 11:16
adriansr
adriansr suggested changes Jul 13, 2022
View reviewed changes
Copy link
Contributor

@adriansr adriansr left a comment

Choose a reason for hiding this comment

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

Just a final adjustment to the docs

x-pack/filebeat/docs/inputs/input-httpjson.asciidoc Outdated
[float]
==== `chain[].while.replace`

Please refer `chain[].step.replace`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR still adds a few "Please refer X" expressions. My understanding is that those are not correct, please correct me if I'm wrong.

I lean towards using "See X" instead of "Please refer to X for more information" as a replacement.

Also, this one uses a `literal` instead of a <<link>>, can it be turned into a link?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adriansr updated the docs as per the suggestions. Please review once.

adriansr
adriansr approved these changes Jul 13, 2022
View reviewed changes
Copy link
Contributor

@adriansr adriansr left a comment

Choose a reason for hiding this comment

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

Question. Other than that LGTM

@ShourieG ShourieG merged commit 8021971 into elastic:main Jul 14, 2022
@BenB196 BenB196 mentioned this pull request Oct 20, 2022
Closed
chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
@ShourieG @adriansr @chrisberkhout
[elastic/beats] [HttpJson] - Improves request chaining with new 'whil…
1912a03 
…e' step & client creation per step (#32222)

* initial commit

* While block introduced in chain config, per step creation of httpclient added as an improvement.

* removed debug code

* added auth inheriting from parent

* updated docs

* made linter recommended fixes

* made recommended changes as per PR suggetions

* added tests

* fixed errors for linting checks

* eliminated specifying nill values

* made till , a mandatory parameter for whileconfig

* Update x-pack/filebeat/input/httpjson/policy.go

Co-authored-by: Adrian Serrano <[email protected]>

* Update x-pack/filebeat/docs/inputs/input-httpjson.asciidoc

Co-authored-by: Adrian Serrano <[email protected]>

* updated docs , made PR fixes

* updated the docs as per suggetions

Co-authored-by: Adrian Serrano <[email protected]>
@chrisberkhout chrisberkhout mentioned this pull request Jan 10, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@efd6 efd6 efd6 left review comments

@adriansr adriansr adriansr approved these changes

@marc-gr marc-gr Awaiting requested review from marc-gr

@P1llus P1llus Awaiting requested review from P1llus

Assignees

@ShourieG ShourieG

Labels
8.4-candidate enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Filebeat][httpjson input] Add status check API calls.
4 participants
@ShourieG @elasticmachine @adriansr @efd6