Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for jmx metrics and prometheus exporter #182

Closed
wants to merge 3 commits into from

Conversation

kempspo
Copy link
Contributor

@kempspo kempspo commented Jun 9, 2024

No description provided.

@cla-bot cla-bot bot added the cla-signed label Jun 9, 2024
@nineinchnick
Copy link
Member

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
Copy link
Contributor Author

kempspo commented Jun 9, 2024

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

Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, thanks for adding tests! If you'll address all comments, please squash commits, keeping logical changes separate:

  • bumping version
  • adding new properties

test.sh Outdated Show resolved Hide resolved
charts/trino/values.yaml Outdated Show resolved Hide resolved
charts/trino/values.yaml Outdated Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
charts/trino/values.yaml Outdated Show resolved Hide resolved
charts/trino/values.yaml Outdated Show resolved Hide resolved
charts/trino/values.yaml Outdated Show resolved Hide resolved
@mosabua
Copy link
Member

mosabua commented Jun 10, 2024

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

@kempspo kempspo force-pushed the main branch 2 times, most recently from 4ca2d58 to e665f52 Compare June 11, 2024 20:14
Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there

charts/trino/templates/tests/test-jmx.yaml Show resolved Hide resolved
charts/trino/values.yaml Outdated Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
test.sh Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
@kempspo
Copy link
Contributor Author

kempspo commented Jun 12, 2024

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
Copy link
Member

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.

Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two last comments, squash commits and I can merge this.

test-values.yaml Outdated Show resolved Hide resolved
charts/trino/templates/servicemonitor.yaml Outdated Show resolved Hide resolved
@nineinchnick
Copy link
Member

@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
Copy link
Member

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
Copy link
Contributor Author

kempspo commented Jun 14, 2024

@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 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants