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

Add an option to report deltas during a flow period instead of cumulative stats #38223

Merged
merged 7 commits into from
Mar 21, 2024
Merged

Add an option to report deltas during a flow period instead of cumulative stats #38223

merged 7 commits into from
Mar 21, 2024

Conversation

kvalliyurnatt
Copy link
Contributor

@kvalliyurnatt kvalliyurnatt commented Mar 7, 2024 •
edited
Loading

Proposed commit message

Currently intermediate flows and final flows report cumulative stats for network.bytes and network.packets. Adding a config option enable_delta_flow_reports that will result in intermediate flows and final flows reporting network.bytes and network.packets for only that corresponding period instead of cumulative.

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.

Use cases

Having delta stats in intermediate flows helps calculating things like total bytes for a specific time window easier without having to look at flows for other time periods to arrive at an accurate answer.

@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 Mar 7, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Mar 7, 2024

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

@kvalliyurnatt kvalliyurnatt mentioned this pull request Mar 7, 2024
Closed
6 tasks
@elasticmachine
Copy link
Collaborator

elasticmachine commented Mar 7, 2024 •
edited
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

  • Duration: 49 min 16 sec

❕ Flaky test report

No test was executed to be analysed.

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

@kvalliyurnatt kvalliyurnatt marked this pull request as ready for review March 7, 2024 21:57
@kvalliyurnatt kvalliyurnatt requested a review from a team as a code owner March 7, 2024 21:57
@elasticmachine
Copy link
Collaborator

Pinging @elastic/sec-linux-platform (Team:Security-Linux Platform)

yspotts
yspotts reviewed Mar 8, 2024
View reviewed changes
packetbeat/flows/worker_test.go
@@ -116,7 +116,7 @@ func TestCreateEvent(t *testing.T) {

// Write the event to disk if -data is used.
if *dataFlag {
event.Fields.Put("@timestamp", common.Time(end)) //nolint:errcheck // Never fails.
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was failing the lint check saying this is not used.

yspotts
yspotts reviewed Mar 8, 2024
View reviewed changes
Copy link
Contributor

@yspotts yspotts left a comment

Choose a reason for hiding this comment

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

overall, looks good to me.

haesbaert
haesbaert approved these changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@haesbaert haesbaert left a comment

Choose a reason for hiding this comment

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

The typo on cummlative as pointed out, other that LGTM!

packetbeat/flows/worker_test.go Show resolved Hide resolved
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @kvalliyurnatt

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @kvalliyurnatt

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @kvalliyurnatt

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @kvalliyurnatt

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @kvalliyurnatt

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @kvalliyurnatt

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @kvalliyurnatt

@elasticmachine
Copy link
Collaborator

elasticmachine commented Mar 20, 2024 •
edited
Loading

💔 Build Failed

Failed CI Steps

History

cc @kvalliyurnatt

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @kvalliyurnatt

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @kvalliyurnatt

@kvalliyurnatt
Copy link
Contributor Author

@haesbaert any idea why the filebeat tests are failing, they don't seem to be related to my changes .

@haesbaert
Copy link
Contributor

@haesbaert any idea why the filebeat tests are failing, they don't seem to be related to my changes .

No idea, I'll ping some people, I don't think it should block this PR though.

@nfritts nfritts merged commit b98b763 into elastic:main Mar 21, 2024
46 of 49 checks passed
@nfritts nfritts added the backport-v8.13.0 Automated backport with mergify label Mar 21, 2024
mergify bot pushed a commit that referenced this pull request Mar 21, 2024
@kvalliyurnatt @mergify
Add an option to report deltas during a flow period instead of cumula…
ee40ea7 
…tive stats (#38223)

* add an option to report delta flows

* adding unit test and doc update

* fix linting issues

* fix imports

* addressing review comments

(cherry picked from commit b98b763)
@mergify mergify bot mentioned this pull request Mar 21, 2024
Merged
6 tasks
norrietaylor pushed a commit that referenced this pull request Mar 21, 2024
@mergify @kvalliyurnatt
Add an option to report deltas during a flow period instead of cumula…
2b50d68 
…tive stats (#38223) (#38537)

* add an option to report delta flows

* adding unit test and doc update

* fix linting issues

* fix imports

* addressing review comments

(cherry picked from commit b98b763)

Co-authored-by: kvalliyurnatt <135071014+kvalliyurnatt@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yspotts yspotts yspotts left review comments

@haesbaert haesbaert haesbaert approved these changes

@andrewkroh andrewkroh Awaiting requested review from andrewkroh

Assignees

@kvalliyurnatt kvalliyurnatt

Labels
8.13-candidate backport-v8.13.0 Automated backport with mergify enhancement Packetbeat Team:Security-Linux Platform Linux Platform Team in Security Solution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kvalliyurnatt @elasticmachine @haesbaert @yspotts @nfritts