fix(chart/opensearch): metricsPort and plugins usage info in values.yaml #593
Conversation
eyenx
requested review from
TheAlgo,
DandyDeveloper,
peterzhuamazon,
prudhvigodithi and
gaiksaya
as code owners
|
Note: We are in the review of an older PR now so will come back to this a little later. |
|
@peterzhuamazon any update here? Thanks! |
|
@peterzhuamazon any update here? |
There was a problem hiding this comment.
Hi @eyenx sorry for the delay we are in a few releases, and just completed 2.18.0 yesterday evening.
charts/opensearch/values.yaml
Outdated
| metricsPort: 9600 | ||
| metricsPort: 9200 |
There was a problem hiding this comment.
The metrics port here is for Performance Analyzer which utilizes the 9600 port
69daf97
There was a problem hiding this comment.
I wonder if we need to add a new var for other metrics?
There was a problem hiding this comment.
@eyenx if we want to use this metricsPort (The metrics port (for Performance Analyzer) that Kubernetes will use for the service) then it should be 9600 right ?
There was a problem hiding this comment.
I will check tomorrow.
There was a problem hiding this comment.
Does the performance Analazer actually expose metrics in prometheus format? If not I would still go for this, ut add 9600 as separate port. How can I enale the performance Analyzer plugin?
There was a problem hiding this comment.
I think when you enable PA you would need to query through 9600
https://opensearch.org/docs/latest/monitoring-your-cluster/pa/index/#example-api-query-and-response
There was a problem hiding this comment.
As mentioned above, I tried curling 9600, but no response. So this is why I went for 9200 at the end. Did anyone ever got this working?
There was a problem hiding this comment.
You would need to separately start the PA process I think.
Not an expert on that tho.
There was a problem hiding this comment.
Okay, after having looked at it together with @cfi2017 we came to the conclusion that the metricsPort has nothing to do with the prometheus metrics but rather is to be used in conjuction with Performance Analayzer.
Therefore I'm changing this PR to make sure the ServiceMonitor is deployed to grab metrics from 9200 (httpPort) and keeping the comment about how to install opensearch-prometheus-exporter.
We tried this locally and it just works.
There was a problem hiding this comment.
Please another review, but waiting for #594 before bumping the version here. @peterzhuamazon @prudhvigodithi
Signed-off-by: Toni Tauro <[email protected]>
…r 9200 if prom-exporter is used Signed-off-by: Toni Tauro <[email protected]>
eyenx
force-pushed
the
fix/chart-opensearch/serviceMonitor-right-port-and-docs
branch
from
76017aa to
f3409a4
Compare
Signed-off-by: Toni Tauro <[email protected]>
|
I would also be very happy to backport this to version |
|
Hi @eyenx , most of the team are on vacation soon and sorry for the delay due to recent 1.3.20 release. I am happy to put this PR on my calendar for next Monday seems like a simple change, but #594 I am afraid I need another maintainer for co-review. I will try to @ maintainers again but most likely that will be delayed until they are back from holiday. Thanks. |
|
Also seems like the changelog part needs some updates. Thanks. |
|
Yes I can add this but it's highly dependant what version will be merged first |
Description
Trying to figure out how to expose metrics, it was obvious that there is the need of installing an additional plugin to be able to expose prometheus metrics over the serviceMonitor. Also the default port exposing the metrics is 9200 and not 9600.
This fixes the default metricsPort and adds a comment about the plugin to be used in
values.yamlIssues Resolved
#590
Check List
For any changes to files within Helm chart directories:
CHANGELOG.mdupdated to reflect changeBy 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.