-
Notifications
You must be signed in to change notification settings - Fork 9
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
Prometheus metrics #144
Prometheus metrics #144
Conversation
src/main/resources/application.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mind enabling the management endpoints by default. Can it be split in a separate configuration? A specific spring profile may do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you gave it a try with the current version in Master ?
Then only endpoints visible with current version of the Mag on the Master are:
PS C:\Windows\System32> curl http://localhost:9090/actuator
{"_links":{"self":{"href":"http://localhost:9090/actuator","templated":false},"health":{"href":"http://localhost:9090/actuator/health","templated":false},"health-path":{"href":"http://localhost:9090/actuator/health/{*path}","templated":true}}}
To have more you have to enable and expose, what we did is a proposal, because, none of the endpoints needed for Kubernetes are exposed by default, to the availability and the readiness.
If you noticed in the PR, maybe it should not be put like that, because you have too many endpoints exposed and that might not be desirable.
For Prometheus we draw your attention that an extra reference was needed in the POM, without that the endpoint will not appear, but how should we take this forward?
We can have a quick synch if that makes sense.
src/main/resources/keystore.jks
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a new keystore in main?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified it also in the last commit, to use the test keystore and not the one in main.
@@ -6,7 +6,7 @@ mag: | |||
client-ssl: | |||
enabled: true | |||
key-store: | |||
path: example-client-certificate.jks | |||
path: src/main/resources/keystore.jks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it not use a test keystore?
5faea83
to
8bb4617
Compare
Hi Quentin, first of all thank you for your time last Friday. |
… endpoints. Enabled in the default profile the kubernertes specific endpoints and removed all the other exposed endpoints.
Fyi, I just did a push in the branch where the changes were requested, can you please have a look at it ? |
This pr fix an issue with the local keystore and add health-checks and prometheus metrics