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] Add http metrics to httpjson input #35392

Merged
merged 30 commits into from
Jun 1, 2023

Conversation

marc-gr
Copy link
Contributor

@marc-gr marc-gr commented May 9, 2023

What does this PR do?

Adds an instrumented roundtripper to HTTPJSON http client to monitor metrics for the input.

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 or CHANGELOG-developer.next.asciidoc.

The resulting metrics look like:

[
    {
        "http": {
            "request": {
                "body": {
                    "size": {
                        "histogram": {
                            "count": 80,
                            "max": 13,
                            "mean": 13,
                            "median": 13,
                            "min": 13,
                            "p75": 13,
                            "p95": 13,
                            "p99": 13,
                            "p999": 13,
                            "stddev": 0
                        }
                    }
                },
                "delete": 0,
                "errors": 0,
                "get": 0,
                "head": 0,
                "options": 0,
                "patch": 0,
                "post": 80,
                "put": 0,
                "total": 80
            },
            "response": {
                "1xx": 0,
                "2xx": 79,
                "3xx": 0,
                "4xx": 0,
                "5xx": 0,
                "body": {
                    "size": {
                        "histogram": {
                            "count": 79,
                            "max": 16,
                            "mean": 16,
                            "median": 16,
                            "min": 16,
                            "p75": 16,
                            "p95": 16,
                            "p99": 16,
                            "p999": 16,
                            "stddev": 0
                        }
                    }
                },
                "errors": 0,
                "total": 79
            },
            "round_trip": {
                "time": {
                    "histogram": {
                        "count": 79,
                        "max": 749746404,
                        "mean": 479335250.3291139,
                        "median": 474980809,
                        "min": 443448808,
                        "p75": 486463110,
                        "p95": 506870389,
                        "p99": 749746404,
                        "p999": 749746404,
                        "stddev": 34458519.317051604
                    }
                }
            }
        },
        "httpjson": {
            "interval": {
                "errors": 0,
                "execution_time": {
                    "histogram": {
                        "count": 79,
                        "max": 750016976,
                        "mean": 479614631.3291139,
                        "median": 475282839,
                        "min": 443708431,
                        "p75": 486757920,
                        "p95": 507214631,
                        "p99": 750016976,
                        "p999": 750016976,
                        "stddev": 34458622.68300466
                    }
                },
                "pages": {
                    "execution_time": {
                        "histogram": {
                            "count": 79,
                            "max": 61675,
                            "mean": 27498.291139240508,
                            "median": 25823,
                            "min": 14148,
                            "p75": 32119,
                            "p95": 39489,
                            "p99": 61675,
                            "p999": 61675,
                            "stddev": 7905.758106870704
                        }
                    },
                    "total": {
                        "histogram": {
                            "count": 0,
                            "max": 0,
                            "mean": 0,
                            "median": 0,
                            "min": 0,
                            "p75": 0,
                            "p95": 0,
                            "p99": 0,
                            "p999": 0,
                            "stddev": 0
                        }
                    }
                },
                "total": 79
            }
        },
        "id": "my-id",
        "input": "httpjson"
    }
]

@elasticmachine
Copy link
Collaborator

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

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels May 9, 2023
@mergify
Copy link
Contributor

mergify bot commented May 9, 2023

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @marc-gr? 🙏.
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

@mergify mergify bot assigned marc-gr May 9, 2023
@marc-gr marc-gr force-pushed the feat/add-httpjson-metrics branch from 922bfd2 to 0e830be Compare May 9, 2023 09:54
@elasticmachine
Copy link
Collaborator

elasticmachine commented May 9, 2023

💚 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: 2023-05-31T15:09:27.941+0000

  • Duration: 75 min 52 sec

Test stats 🧪

Test Results
Failed 0
Passed 2927
Skipped 175
Total 3102

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

@marc-gr marc-gr marked this pull request as draft May 9, 2023 12:05
@marc-gr marc-gr marked this pull request as ready for review May 10, 2023 09:44
@andrewkroh andrewkroh self-requested a review May 10, 2023 21:16
@P1llus
Copy link
Member

P1llus commented May 11, 2023

Added some comments from slack here to share, just a few thoughts after understanding more about how each are used. I won't cover the field namings as I feel @andrewkroh has a much better overview on that, I have no specific opinion on that part.

  1. From my understanding, we manage to differentiate request/response errors with the custom RoundTripper, but the httpjson error counter includes request, response and "other" errors.
    While it could be a topic for later, since it might require too much work, it would be nice if the httpjson error counter only counted non request-response errors, to make it less confusing. But as a workaround for now, we can simply subtract the request/response errors with the other to get a better picture.

  2. I am a bit unsure if the pages total needs to be a histogram, since the other totals are not, but I have no specific opinion on that.

  3. Chains are not included, which I feel is not really required for an MVP, seeing that is barely used for now, and won't affect our integration coverage much at all. Maybe at a later date if we still feel its necessary (v3?)

Except that it looks nice! Really looking forward to start visualizing this! :)

Copy link
Member

@rdner rdner left a comment

Choose a reason for hiding this comment

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

  1. Do you think we have sufficient test coverage to make sure this change does not break anything?

  2. Do we need a new test to make sure the metrics are collected?

@mergify
Copy link
Contributor

mergify bot commented May 19, 2023

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 feat/add-httpjson-metrics upstream/feat/add-httpjson-metrics
git merge upstream/main
git push upstream feat/add-httpjson-metrics

@marc-gr
Copy link
Contributor Author

marc-gr commented May 26, 2023

  1. Do you think we have sufficient test coverage to make sure this change does not break anything?

The input has quite a lot of testing to be confident about it not breaking it.

  1. Do we need a new test to make sure the metrics are collected?

Added a test to validate metric collection.

@marc-gr marc-gr requested a review from rdner May 26, 2023 07:34
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.

5 participants