feat: add Elastic metrics provider #3741
Conversation
mowies
changed the title
|
also, if you can, please provide some unit tests for your new code :) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3741 +/- ##
===========================================
- Coverage 74.72% 51.77% -22.95%
===========================================
Files 225 42 -183
Lines 10160 2445 -7715
===========================================
- Hits 7592 1266 -6326
+ Misses 2198 1021 -1177
+ Partials 370 158 -212 see 196 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. |
| ctx, cancel := context.WithTimeout(ctx, 20*time.Second) | ||
| defer cancel() | ||
|
|
||
| result, err := r.runElasticQuery(ctx, metric.Spec.Query, time.Now().Add(-30*time.Minute), time.Now()) |
There was a problem hiding this comment.
do we always want to have exactly the last 30min time slot?
There was a problem hiding this comment.
for now it's hardcoded but we can make it passed dynamically btw looking for your feedback on the right way?
There was a problem hiding this comment.
let's make it dynamic please
| } | ||
|
|
||
| func (r *KeptnElasticProvider) extractMetric(result map[string]interface{}) (string, error) { | ||
| hits, ok := result["hits"].(map[string]interface{}) |
There was a problem hiding this comment.
does the response have a standardized format? maybe we can create a struct and unmarshall to it
There was a problem hiding this comment.
okay let me check on it
|
Hey @mowies @odubajDT I have created test cases for the methods in elk provider. In short-
|
Signed-off-by: Bharadwajshivam28 <[email protected]>
Signed-off-by: Bharadwajshivam28 <[email protected]>
Signed-off-by: Bharadwajshivam28 <[email protected]>
Signed-off-by: Bharadwajshivam28 <[email protected]>
…act metric method Signed-off-by: Bharadwajshivam28 <[email protected]>
Signed-off-by: Bharadwajshivam28 <[email protected]>
Bharadwajshivam28
force-pushed
the
feat/elk
branch
from
5cdfdad to
8daeb28
Compare
|
| elasticURL, | ||
| }, | ||
| } | ||
| es, err := elastic.NewClient(cfg) |
There was a problem hiding this comment.
does elastic have a mock library? if yes, let's try to use it for testing. if not, maybe we can create an interface and try to mock the elastic to improve the testing, WDYT?
| _, _ = w.Write([]byte(response)) | ||
| })) | ||
| defer server.Close() | ||
|
|
There was a problem hiding this comment.
let's try to mock the elastic to make the testing easier for multiple testing scenarios
There was a problem hiding this comment.
Thanks for the contributions, please see the additional comments on the code. One question tho, did you manually tried the implementation with Keptn? Seems to me that currently won't work, as the new provider is not present in the list of supported providers. If possible please add it and try it out manually.
Also additional question, would it be possible to create a full e2e test for the provider? Would be awesome if we could be able to test it as part of our CI
ya I can create full e2e test for the provider btw can you brief me above lines I didn't understood this "manually tried the implementation with Keptn" |
What I mean is using the elastic metric provider with Keptn, this means setting up an application, monitor it and fetch some data with the elastic metrics provider, used the data for Evaluations and then use Keptn to deploy the application. |
The thing is I am not familiar with elk much so can it be like once pr is merged then users who use elk can use it? I am familiar with code part implementation and till now I feel the provider is mostly correct and the test cases also just a minor change required. |
I understand, but part of the implementation should be a e2e/integration test, which will require to deploy the provider and test the correct functioning of the provider with Keptn. In this case I would suggest getting in contact with the author of the issue regarding some more guidance about deploying and using the provider. Therefore we can verify the correct functionality of the provider and potentially also add some configurable parameters, that are needed in the provider. Another option for the e2e test might be to use mock-server for mocking the responses of the provider, like it's done here. Therefore there is no need for you to actually deploy the provider locally. Please have a look at this option. In that case, we can maybe ask the author of the issue @arkaonline to test the implementation for us once it's ready and merged before actually releasing the feature. But the e2e test mocking the provider is still needed and the PR shouldn't be merged without it. |
Okay so let me go through these and first create the e2e test mocking feature. Also yes the issue author can help us to test the full provider. |
|
@Bharadwajshivam28 @odubajDT I can surely help you test it out. |
Thank you! Yes you are right, we are missing multiple places now. When the implementation will be ready for testing, we will reach out to you. :-) |
so for now I think I should make the e2e tests ready and then @arkaonline can help us in testing the provider once the code is merged? CC- @odubajDT |
First, the implementation needs to be finished and covered by unit tests, afterwards we should aim for a integration/e2e test with using the mock server. When this is finished, we can ask @arkaonline to test Keptn with the new provider and afterwards we can proceed with merging the PR. |
Okay cool so I will Focus on the unit tests and e2e test with the mock server. Also can you please share some information to start with the e2e test? |
You should use chainsaw and mock-server how it's done for example here |
Okay so this for the e2e test right? |
Hey @odubajDT I saw the e2e test files you shared so I had one question: can you please give me information about getting started on e2e tests implementation for the provider I am creating? |
Sure, please have a look at the testing framework we are using and also on mock-server. With the example I pointed you into that should get you going |
|
@Bharadwajshivam28 any updates on this? |
I am working on this and will finish the logic and unit tests by today and for the e2e tests I will make it finish in upcoming 1-2 days. The provider is completed and the unit test also I need to just work on the e2e tests |
| } `json:"hits"` | ||
| } | ||
|
|
||
| func NewElasticProvider(log logr.Logger, k8sClient client.Client, elasticURL string) (*KeptnElasticProvider, error) { |
There was a problem hiding this comment.
where is this code actually used? Currently it isn't used anywhere, therefore it has no impact on the functionality (dead code). Please add the appropiate changes in the providers factory and also in the KeptnMetricsProvider CRD
|
This PR has been automatically marked as stale because it has not had recent activity. It will be |
Description
fixes #3727
Hey @mowies I have started creating custom KeptnMetricsProvider for Elastic and now I have integrated some methods.
For now the methods purpose are-