cloud-bulldozer / go-commons

Code repository with all go common packages and libraries
Apache License 2.0
4 stars 9 forks source link

Handling redundant metrics for indexing #35

Closed vishnuchalla closed 7 months ago

vishnuchalla commented 8 months ago

Type of change

Description

Added code to check duplicate docs during indexing.

Related Tickets & Documents

Checklist before requesting a review

Testing

Tested integrating with kube-burner and verified the changes.

codecov[bot] commented 8 months ago

Codecov Report

Merging #35 (71e220d) into main (dc2da49) will increase coverage by 1.37%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #35      +/-   ##
==========================================
+ Coverage   83.33%   84.70%   +1.37%     
==========================================
  Files           6        6              
  Lines         312      340      +28     
==========================================
+ Hits          260      288      +28     
  Misses         38       38              
  Partials       14       14              
Files Coverage Δ
indexers/elastic.go 75.55% <100.00%> (+4.50%) :arrow_up:
indexers/opensearch.go 72.22% <100.00%> (+5.11%) :arrow_up:
github-actions[bot] commented 8 months ago

Code coverage: 88.2%

rsevilla87 commented 8 months ago

Im not a fan of this neither this PR nor https://github.com/cloud-bulldozer/kube-burner/pull/501, they don't actually fix anything, I think they increase the code complexity needlessly to skip indexing, only 7 repeated documents, https://github.com/cloud-bulldozer/kube-burner/blob/master/cmd/kube-burner/ocp-config/metrics-report.yml#L232-L259

If a user is not using the indexing feature correctly, it should be up to him to realize about it, we shouldn't extra add checks in code to prevent a user using kube-burner incorrectly.

rsevilla87 commented 8 months ago

In addition, I think fixing this "issue" shouldn't affect the go-commons code, this check can be easily implemented by looking for duplicates in the metrics specified in the metric profiles files.

vishnuchalla commented 8 months ago

In addition, I think fixing this "issue" shouldn't affect the go-commons code, this check can be easily implemented by looking for duplicates in the metrics specified in the metric profiles files.

This change is not made keeping only kube-burner in mind. Its just a sanity check to prevent duplicates and I feel go-commons is correct place to do it as we have multiple tools using it.

vishnuchalla commented 8 months ago

Im not a fan of this neither this PR nor cloud-bulldozer/kube-burner#501, they don't actually fix anything, I think they increase the code complexity needlessly to skip indexing, only 7 repeated documents, https://github.com/cloud-bulldozer/kube-burner/blob/master/cmd/kube-burner/ocp-config/metrics-report.yml#L232-L259

If a user is not using the indexing feature correctly, it should be up to him to realize about it, we shouldn't extra add checks in code to prevent a user using kube-burner incorrectly.

This is not about completely fixing any issue. It about adding all the possible sanity checks from the code standpoint to prevent unintended abuse/misuse cases. When we have a harmless possibility to add these checks then why not just add them?

vishnuchalla commented 8 months ago

just a gentle reminder on this PR

vishnuchalla commented 7 months ago

Merging these code changes as they are not any harm and just a few sanity checks to add an extra layer of validation for indexers.