-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Introducing micrometer/prometheus metric collection #105
Conversation
@@ -72,17 +72,42 @@ ENV SPRING_PROFILES_ACTIVE docker,mysql | |||
In the `mysql section` of the `application.yml` from the [Configuration repository], you have to change | |||
the host and port of your MySQL JDBC connection string. | |||
|
|||
## Custom metrics monitoring | |||
|
|||
@todo Add default custom dashboards to grafana |
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.
maybe in a docs/ folder or a gh-pages branch?
I have also to reupload the Spring Petcilinic screenshot of thr readme.md. The link is broken: https://cloud.githubusercontent.com/assets/838318/19653851/61c1986a-9a16-11e6-8b94-03fd7f775bb3.png
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 agree, screenshots of the default dashboard setup in the meantime until the custom metrics dashboard can be built out and included in a custom image build.
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.
For information, docs/
folder has been introduced in the PR #106
|
||
@todo Add default custom dashboards to grafana | ||
|
||
Grafana and Prometheus are included in the `docker-compose.yml` configuration, and the public facing applications have been instrumented with [MicroMeter](https://micrometer.io) to collect JVM and custom business metrics. |
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.
For development, does the application work if Grafana and Prometheus are not started?
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.
There is no dependency on those services at runtime. This implementation produces an additional endpoint on the actuator which is scraped by prometheus.
@@ -0,0 +1,32 @@ | |||
# my global config |
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.
Is there any configuration to commit in the config git repository; https://github.com/spring-petclinic/spring-petclinic-microservices-config ?
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.
Nice catch! There is. I'll fork that and create a PR for those changes.
@mszarlinski did you have time to review this PR? |
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.
Thanks for your amazing work 👍 please answer my comments
|
||
/** | ||
* Create Owner | ||
*/ | ||
@PostMapping | ||
@ResponseStatus(HttpStatus.CREATED) | ||
public void createOwner(@Valid @RequestBody Owner owner) { | ||
registry.counter("create.owner").increment(); |
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.
What about using @Timed? If we use it instead of manually incrementing metrics, tests will be alse simplified. IMHO there is little profit in measuring number of requests - Prometheus has metrics for RPS etc.)
https://spring.io/blog/2018/03/16/micrometer-spring-boot-2-s-new-application-metrics-collector
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.
Agreed. I'm delivering the workshop today so once that's done I'll make this change.
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.
@notsureifkevin do you any time to make the change?
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.
https://micrometer.io/docs/concepts#_the_code_timed_code_annotation
It's not clear to me how to use this annotation and I'm constrained for time as-is. You're welcome to use merge this PR as-is, attempt to make these modifications yourself, or drop it completely. I've rebased atop master, so it should be good to go as-is.
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.
@mszarlinski I propose to merge this PR and create an issue for improvment. I could work on 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.
I've created the #120 in order to merge this PR then improve it.
I've tested the |
In order to easier test all metrics dashboard we have (Promotheus/Grafana/OpenTracing/Hystrix Dashboard), I wonder if you could add some load tests with JMeter or Gatlin. |
I built a simple load generation script with locust/python. I'm happy to share that. In order to test the create functionality there needs to be an additional RE: the other changes, I simply haven't had the time and I apologize. I'll make it a point to free up some cycles for this over the next few weeks. |
I don't know Locust but why not. It could be great. |
What is a point in making performance tests on demo application? I think that simple Gatling simulation to make a couple of requests would be sufficient. By the way, Grafana dashboard looks pretty sweet! |
Not really for performance tests, but in order to simulate some user traffic and to have pretty dashboard :) |
I would like to keep it simple. Maybe bash script or simple Gatling simulation would be enough to feed metrics? |
I'm totally agree with you. We have to keep the project simple. Moreover this script. A single script file and a comment line in the |
Hi @notsureifkevin. Could we finish this PR? A least, some comments from @mszarlinski ? |
Hello, I've commented on the outstanding issue, it appears some additional configuration/changes would be required to make use of his suggestion; I feel the current implementation, while not the most pragmatic, is more than adequate for a usage example. Of course, if someone else is willing to implement/build upon the changes I've made I'd more than welcome the knowledge and experience. |
I have built a dashboard for this application here: https://gist.githubusercontent.com/notsureifkevin/104adca1ccd4b96937738f9bfbb6ba46/raw/1fddbe7eeebcb57dfb25d0407161710928d9b52f/petclinic.json |
https://gitlab.com/kc_wrhse/spring-petclinic-kubernetes/tree/master/scripts/spc-load |
@notsureifkevin I have a question. In your custom grafana dashboard, what means the
I don't see that attribut in data result:
|
@arey this is an artifact of the work I was doing in a workshop/lab which included multiple namespaces - I believe you could remove that attribute and it would work as intended |
Introducing Micrometer Prometheus metric collection
related to #103 - tested using jvm8/10