- Notifications
You must be signed in to change notification settings - Fork545
Comments
Spring boot package [Threads - data stream]#2981
Conversation
elasticmachine commentedApr 3, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
💚 Build Succeeded
Expand to view the summaryBuild stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
elasticmachine commentedApr 3, 2022
Pinging @elastic/integrations (Team:Integrations) |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| - name: threading | ||
| type: group | ||
| fields: | ||
| - name: current_thread |
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 does thecurrent_thread mean in terms of Jolokia request? Which one is the current thread? Should we skip 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.
so this current_thread should be current running thread , it has different values than threads. I think we should keep this.
kush-elastic left a comment
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.
@sunny-elastic that's all from my end, left some comments.
@mtojek, please take a look and feel free to drop in suggestions/questions.
packages/spring_boot/manifest.yml Outdated
| title: Collect Spring Boot metrics of Memory and Threading. | ||
| description: Collecting metrics from Spring Boot of Memory and Threading. |
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.
can we update this to something like following:
| title:Collect Spring Boot metricsof Memory and Threading. | |
| description:Collecting metrics from Spring Boot of Memory and Threading. | |
| title:Collect Spring Boot metricsusing Jolokia. | |
| description:Collecting metrics from Spring Boot of Memory and Threading using Jolokia. |
so in future if there are anymore data-stream that uses same jolokia input, we don't have provide all the types in title.
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.
yes done
| - name: daemon_thread_count | ||
| type: long | ||
| description: Current number of live daemon threads | ||
| - name: object_monitor_usage_supported | ||
| type: boolean | ||
| description: Object monitor usage support | ||
| - name: peak_thread_count | ||
| type: long | ||
| description: Peak thread count to the current number of live threads | ||
| - name: synchronizer_usage_supported | ||
| type: boolean | ||
| description: Show the synchronizer usage support | ||
| - name: total_started_thread_count | ||
| type: long | ||
| description: Total number of threads created and also started since the Java virtual machine 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.
can we do this?
| -name:daemon_thread_count | |
| type:long | |
| description:Current number of live daemon threads | |
| -name:object_monitor_usage_supported | |
| type:boolean | |
| description:Object monitor usage support | |
| -name:peak_thread_count | |
| type:long | |
| description:Peak thread count to the current number of live threads | |
| -name:synchronizer_usage_supported | |
| type:boolean | |
| description:Show the synchronizer usage support | |
| -name:total_started_thread_count | |
| type:long | |
| description:Total number of threads created and also started since the Java virtual machine started | |
| -name:daemon | |
| type:long | |
| description:Current number of live daemon threads | |
| -name:object_monitor_usage_supported | |
| type:boolean | |
| description:Object monitor usage support | |
| -name:peak | |
| type:long | |
| description:Peak thread count to the current number of live threads | |
| -name:synchronizer_usage_supported | |
| type:boolean | |
| description:Show the synchronizer usage support | |
| -name:total_started | |
| type:long | |
| description:Total number of threads created and also started since the Java virtual machine 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.
yeah make sense, its updated
kush-elastic left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
LGTM! Let's wait for other's review.
mtojek left a comment
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.
Few comments on my side.
- I'm not sure why do we need to collect these booleans.
- I think we need another level
spring_boot.threading.threads.*for metrics likecurrentandstarted(instead of daemon and total_started).
| "allocated_memory": { | ||
| "enabled": true, | ||
| "supported": true | ||
| }, | ||
| "contention_monitoring": { | ||
| "enabled": false, | ||
| "supported": true | ||
| }, |
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 do these metrics mean? They seem to me more like a configuration.
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.
its updated now and removed the unwanted ones
| "cpu_time": { | ||
| "enabled": true, | ||
| "supported": true | ||
| }, |
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.
Same story here. As a user, I expectedcpu_time but got some booleans.
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.
we updated the structure and kept only relevant fields
| "cpu_time_supported": true, | ||
| "user_time": 470000000 | ||
| }, | ||
| "daemon": 16, |
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 does this metric mean?
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 daemon ? its number of live daemon threads.
| "user_time": 470000000 | ||
| }, | ||
| "daemon": 16, | ||
| "object_monitor_usage_supported": true, |
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.
Boolean?
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.
its updated
| "daemon": 16, | ||
| "object_monitor_usage_supported": true, | ||
| "peak": 20, | ||
| "synchronizer_usage_supported": true, |
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.
Boolean?
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.
its updated
| "object_monitor_usage_supported": true, | ||
| "peak": 20, | ||
| "synchronizer_usage_supported": true, | ||
| "total_started": 23 |
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.
total_started what?
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.
Total number of threads created and also started since the Java virtual machine started. Updated this hierarchy in structure
mtojek left a comment
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.
requested changes
sunny-elastic commentedApr 26, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@mtojek is this following structure looks good to you, since only kept relevant fields which has meaningful values to collect Here if you see thecurrent object it only specifies the details regarding current thread, butcount , daemon and total started are referring to multiple threads so coming underthreads object. And it has appropriate description given in fields.yml |
mtojek commentedApr 26, 2022
Yes, it's a better fit. I'm wondering if we improve it more by applying thisguidance. Then:
|
sunny-elastic commentedApr 26, 2022
yeah but |
mtojek commentedApr 26, 2022
Ok, let's just change the |
elasticmachine commentedApr 27, 2022
🌐 Coverage report
|
sunny-elastic commentedApr 27, 2022
Yeah its updated. Thanks! |
Uh oh!
There was an error while loading.Please reload this page.
What does this PR do?
Checklist
changelog.ymlfile.manifest.ymlfile to point to the latest Elastic stack release (e.g.^8.0.0).How to test this PR locally
Note: We have covered dashboards and the visualisations for all data streams of spring boot into separate PR. Also Kibana version will be updated to 8.1.0 in manifest.yml after testing this integration on 8.1.0.