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

[Feature/Extensions] Profile detector #882

Conversation

vibrantvarun
Copy link
Member

@vibrantvarun vibrantvarun commented Apr 27, 2023

Description

Profile Detector API's for extensions

Issues Resolved

381

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.

@vibrantvarun vibrantvarun changed the title Profile detector [Feature/Extensions] Profile detector Apr 27, 2023
Signed-off-by: Varun Jain <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2023

Codecov Report

Merging #882 (5619241) into feature/extensions (0892126) will decrease coverage by 0.45%.
The diff coverage is 1.85%.

📣 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

Impacted file tree graph

@@                   Coverage Diff                    @@
##             feature/extensions     #882      +/-   ##
========================================================
- Coverage                 34.95%   34.50%   -0.45%     
+ Complexity                 1888     1867      -21     
========================================================
  Files                       300      300              
  Lines                     17863    17875      +12     
  Branches                   1862     1862              
========================================================
- Hits                       6244     6168      -76     
- Misses                    11171    11260      +89     
+ Partials                    448      447       -1     
Flag Coverage Δ
plugin 34.50% <1.85%> (-0.45%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...va/org/opensearch/ad/AnomalyDetectorExtension.java 0.00% <ø> (ø)
...ensearch/ad/rest/RestGetAnomalyDetectorAction.java 0.00% <0.00%> (ø)
...ain/java/org/opensearch/ad/task/ADTaskManager.java 0.07% <0.00%> (+<0.01%) ⬆️
...rch/ad/transport/ADTaskProfileTransportAction.java 0.00% <0.00%> (ø)
...pensearch/ad/transport/ProfileTransportAction.java 0.00% <0.00%> (ø)
...search/ad/transport/RCFPollingTransportAction.java 0.00% <0.00%> (-83.73%) ⬇️
...rg/opensearch/ad/AnomalyDetectorProfileRunner.java 8.36% <25.00%> (-0.06%) ⬇️

... and 7 files with indirect coverage changes

@vibrantvarun vibrantvarun marked this pull request as ready for review April 28, 2023 16:42
@vibrantvarun vibrantvarun requested review from a team, dbwiddis, owaiskazi19 and joshpalis April 28, 2023 16:42
Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

I'm curious where these routes were previously defined?

Signed-off-by: Varun Jain <[email protected]>
@joshpalis joshpalis self-requested a review April 28, 2023 18:53
Signed-off-by: Varun Jain <[email protected]>
Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

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

Will wait for @vibrantvarun to paste the API response of all the profile detector APIs on the issue

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

LGTM!

@vibrantvarun
Copy link
Member Author

All Api's are tested and responses has been added in the comments of the issue.
cc: @owaiskazi19

Signed-off-by: Varun Jain <[email protected]>
import org.opensearch.ad.transport.StopDetectorTransportAction;
import org.opensearch.ad.transport.ValidateAnomalyDetectorAction;
import org.opensearch.ad.transport.ValidateAnomalyDetectorTransportAction;
import org.opensearch.ad.transport.*;
Copy link
Member

Choose a reason for hiding this comment

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

Pasting the usual comment: 1, 2

Copy link
Member Author

Choose a reason for hiding this comment

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

Took care off

new ArrayList<>(List.of(new ADTaskProfileNodeResponse(sdkClusterService.localNode(), adTaskProfile, remoteAdVersion))),
new ArrayList<>()
)
);
Copy link
Member

@owaiskazi19 owaiskazi19 May 1, 2023

Choose a reason for hiding this comment

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

I see these new changes are added now and were not there before because all of the APIs of profile detectors were not tested. Let's test all the APIs from next time to avoid such misses.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I acknowledge that.

Signed-off-by: Varun Jain <[email protected]>
@owaiskazi19 owaiskazi19 merged commit 5dc8edb into opensearch-project:feature/extensions May 2, 2023
@vibrantvarun vibrantvarun deleted the Profile_Detector branch May 2, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants