search

trinodb / charts

Apache License 2.0
151 stars 173 forks source link

Add support for jmx metrics and prometheus exporter #182

Closed kempspo closed 5 months ago

nineinchnick commented 5 months ago

I guess this would replace #140 and #135. Please think about how these changes can be tested - if we're adding resource manifests for external services, we need to deploy them too in the CI to make sure everything works as expected.

kempspo commented 5 months ago

Added the extra services to the CI, but not sure if that's the best way.

mosabua commented 5 months ago

I dont have time to really review but just wanted to mention that this should all be optional and disabled by default.

kempspo commented 5 months ago

I realized I was trying to modify the test.sh to work on repeated runs when cleanup is skipped. But, two things would stop it even before it gets to the Prometheus items

  1. kubectl create namespace would fail if namespace exists
  2. kubectl create secret would fail if secret exists in namespace I can do a follow up PR to fix these.
nineinchnick commented 5 months ago

I realized I was trying to modify the test.sh to work on repeated runs when cleanup is skipped.

By default, the namespace is random. If you specify a namespace, you're supposed to clean it up manually. But we certainly could ignore the existing namespace and the secret, since it does make sense to avoid using a random namespace when debugging issues locally. Thanks for being patient with all the issues here.

nineinchnick commented 5 months ago

@kempspo I applied some minor changes and opened #183, preserving your authorship of commits. I hope you don't mind.

I rewrote the tests in Python. I think they didn't work, the test container might have completed successfully even if the commands inside failed. Took me a bit to get it right, that's why I know it works now. BTW I removed the release label from the service monitor and instead added prometheus.prometheusSpec.serviceMonitorSelectorNilUsesHelmValues=false when deploying the operator, so it can grab any service monitor.

nineinchnick commented 5 months ago

BTW I could have pushed my changes here if you've opened the PR from any other branch than main, since it's protected.

kempspo commented 5 months ago

@kempspo I applied some minor changes and opened #183, preserving your authorship of commits. I hope you don't mind.

I rewrote the tests in Python. I think they didn't work, the test container might have completed successfully even if the commands inside failed. Took me a bit to get it right, that's why I know it works now. BTW I removed the release label from the service monitor and instead added prometheus.prometheusSpec.serviceMonitorSelectorNilUsesHelmValues=false when deploying the operator, so it can grab any service monitor.

No worries. Those changes make sense to me.

Yeah I realized just now that I've been working on main 😅