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

Fix ShardEvents and ShardBulkDocs null metrics #283

Merged
merged 5 commits into from
Jan 27, 2023

Conversation

Tjofil
Copy link
Contributor

@Tjofil Tjofil commented Jan 26, 2023

Signed-off-by: Filip Drobnjakovic [email protected]

Is your feature request related to a problem? Please provide an existing Issue # , or describe.
Issue #251

Describe the solution you are proposing
Did a revert to a previous known functioning point. Both ShardEvents and ShardBulkDocs should now display correct, non-null values.
Currently tested only by hand comparing responses of the patched and non-patched docker OS cluster.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@Tjofil Tjofil requested a review from a team January 26, 2023 21:02
@kkhatua
Copy link
Member

kkhatua commented Jan 26, 2023

@Tjofil is there a test to verify this? Or did you repro in the original docker image and verified it as fixed in the patched docker image?
Would be good to know why it broke and perhaps add a test around this. (More tests for other metrics can be added in the future).

@Tjofil
Copy link
Contributor Author

Tjofil commented Jan 26, 2023

Hey @kkhatua
Changed the PR comment to describe the current level of testing. I agreed with @khushbr to fix it now and have it ready for the next release and maybe do further investigation later, so I just made a init PR and am waiting for her comment on the next steps and potential testing. If you have some suggestions I'd be happy to add them.

@khushbr
Copy link
Collaborator

khushbr commented Jan 26, 2023

Hey @kkhatua Changed the PR comment to describe the current level of testing. I agreed with @khushbr to fix it now and have it ready for the next release and maybe do further investigation later, so I just made a init PR and am waiting for her comment on the next steps and potential testing. If you have some suggestions I'd be happy to add them.

Thank you @Tjofil for the quick fix. Let us add a unit test for shardBulk under ShardStateMetricsSnapshotTest.java with this PR.

Next, we can further beef up our unit test as part of Issue #284

Signed-off-by: Filip Drobnjakovic <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2023

Codecov Report

Merging #283 (f03a8f0) into main (4734d6d) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head f03a8f0 differs from pull request most recent head 02e13ea. Consider uploading reports for the commit 02e13ea to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main     #283      +/-   ##
============================================
+ Coverage     71.93%   71.97%   +0.04%     
- Complexity     2998     3003       +5     
============================================
  Files           380      380              
  Lines         18984    18984              
  Branches       1463     1463              
============================================
+ Hits          13656    13664       +8     
+ Misses         4725     4720       -5     
+ Partials        603      600       -3     
Impacted Files Coverage Δ
...ceanalyzer/reader/ShardRequestMetricsSnapshot.java 99.65% <100.00%> (ø)
...nsearch/performanceanalyzer/rca/RcaController.java 81.05% <0.00%> (-1.77%) ⬇️
...erformanceanalyzer/rca/framework/core/RcaConf.java 55.94% <0.00%> (-1.40%) ⬇️
...formanceanalyzer/reader/RequestEventProcessor.java 87.14% <0.00%> (+1.42%) ⬆️
...nalyzer/rca/net/handler/PublishRequestHandler.java 77.55% <0.00%> (+4.08%) ⬆️
...formanceanalyzer/PerformanceAnalyzerWebServer.java 74.62% <0.00%> (+5.97%) ⬆️
...h/performanceanalyzer/reader/OSEventProcessor.java 81.94% <0.00%> (+6.94%) ⬆️
.../org/opensearch/performanceanalyzer/core/Util.java 70.83% <0.00%> (+8.33%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Filip Drobnjakovic <[email protected]>
Signed-off-by: Filip Drobnjakovic <[email protected]>
@kkhatua kkhatua self-requested a review January 27, 2023 20:27
@khushbr khushbr merged commit 8f5d1f1 into opensearch-project:main Jan 27, 2023
@opensearch-trigger-bot
Copy link

The backport to 1.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.x 1.x
# Navigate to the new working tree
cd .worktrees/backport-1.x
# Create a new branch
git switch --create backport/backport-283-to-1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 8f5d1f1cb2525cd165a1f4d4beccb834b9ad6469
# Push it to GitHub
git push --set-upstream origin backport/backport-283-to-1.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.x

Then, create a pull request where the base branch is 1.x and the compare/head branch is backport/backport-283-to-1.x.

opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 27, 2023
* Fix ShardEvents and ShardBulkDocs null metrics

---------

Signed-off-by: Filip Drobnjakovic <[email protected]>
(cherry picked from commit 8f5d1f1)
@khushbr khushbr added v2.5.0 'Issues and PRs related to version v2.5.0' and removed v2.5.0 'Issues and PRs related to version v2.5.0' labels Jan 27, 2023
khushbr pushed a commit that referenced this pull request Feb 2, 2023
* Fix ShardEvents and ShardBulkDocs null metrics

---------

Signed-off-by: Filip Drobnjakovic <[email protected]>
(cherry picked from commit 8f5d1f1)

Co-authored-by: Filip Drobnjaković <[email protected]>
@systemosyn
Copy link

Hey @Tjofil and @khushbr,
Do you think this issue was the root cause of this one: opensearch-project/performance-analyzer#77 (comment)
Where I saw that only the shabulk started event are written in the EventLog Files. If yes, maybe we could close this issue as well, right?

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