search

opensearch-project / performance-analyzer

📈 Get detailed performance metrics from your cluster independently of the Java Virtual Machine (JVM)
https://opensearch.org/docs/latest/monitoring-plugins/pa/index/
Apache License 2.0
31 stars 66 forks source link

Fixed the bug in CacheConfigMetricsCollector #657

Closed atharvasharma61 closed 3 weeks ago

atharvasharma61 commented 1 month ago

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

Metrics are being emitted as expected:

^cache_config
{"current_time":1717142304989}
{"CacheType":"field_data_cache","Cache_MaxSize":-1}
{"CacheType":"shard_request_cache","Cache_MaxSize":10737418}$

Describe the solution you are proposing Tiered Caching has introduced a new ICache interface which has an OnHeap implementation which actually stores the Cache field now. We had to update the logic to how to get the cache field. Relevant PR: https://github.com/opensearch-project/OpenSearch/pull/10753

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

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.

atharvasharma61 commented 4 weeks ago

Test failures will be addressed in the following PR: https://github.com/opensearch-project/performance-analyzer/pull/651

Gaganjuneja commented 4 weeks ago

@atharvasharma61 thanks for raising this. Could you please add the following details here?

  1. The changes in the core has introduced this issue. As far as I understood there is a level of nesting being introduced with tiered cache. Please provide the changes for reference.
  2. Please add the test case to capture such issues in the build iteslf.
atharvasharma61 commented 4 weeks ago

@atharvasharma61 thanks for raising this. Could you please add the following details here?

1. The changes in the core has introduced this issue. As far as I understood there is a level of nesting being introduced with tiered cache. Please provide the changes for reference.

2. Please add the test case to capture such issues in the build iteslf.

Hi Gagan. Thanks for responding.

  1. I have added the relevant PR in the description.
  2. This issue already got caught in failed UTs for the corresponding collector. So this case is also covered.
Gaganjuneja commented 4 weeks ago

@atharvasharma61 thanks for raising this. Could you please add the following details here?

1. The changes in the core has introduced this issue. As far as I understood there is a level of nesting being introduced with tiered cache. Please provide the changes for reference.

2. Please add the test case to capture such issues in the build iteslf.

Hi Gagan. Thanks for responding.

  1. I have added the relevant PR in the description.
  2. This issue already got caught in failed UTs for the corresponding collector. So this case is also covered.

How come it got released then?

atharvasharma61 commented 4 weeks ago

@atharvasharma61 thanks for raising this. Could you please add the following details here?

1. The changes in the core has introduced this issue. As far as I understood there is a level of nesting being introduced with tiered cache. Please provide the changes for reference.

2. Please add the test case to capture such issues in the build iteslf.

Hi Gagan. Thanks for responding.

  1. I have added the relevant PR in the description.
  2. This issue already got caught in failed UTs for the corresponding collector. So this case is also covered.

How come it got released then?

Looks like it is a miss. UT has been failing for quite some time. PR from 3 months ago: https://github.com/opensearch-project/performance-analyzer/actions/runs/8379459182/job/22946424856