Skip to content

gcPauses -> jvm.gc.pause, memory -> jvm.memory.used #875

New issue

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

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

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from

Conversation

BoykoAlex
Copy link
Contributor

  • Metric name changes: gcPauses -> jvm.gc.pause, memory -> jvm.memory.used. Are those metric names different for other apps?
  • Constants for metric names and NPE-safe string equals checks
  • Measurements value as Double rather than Long

@vudayani-vmw please have a look at this PR and provide feedback.

@@ -232,7 +236,7 @@ private void scheduleRefresh(ProgressTask progressTask, SpringProcessParams spri

try {
progressTask.progressEvent(progressMessage);
if(endpoint.equals("metrics") && metricName.equals("memory")) {
if(METRICS.equals(endpoint) && MEMORY.equals(metricName)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The metricName passed from the API call here is ("memory", "gcPauses"). This is to identify the type of metric and not the actual metricName used for the actuator call. (Probably I should have named it metricType. But I wanted to keep the metricQuery more generic to be used for other metric calls.

The getMemoryMetrics function is where the metric names ("jvm.memory.used, jvm.memory.max") are passed.

Also, with the current implementation, the retrieveLiveMemoryMetricsData has the logic to fetch the heap and non-heap memory metrics together.

SpringProcessGcPausesMetricsLiveData data = connectorService.getGcPausesMetricsLiveData(processKey);
return CompletableFuture.completedFuture(data.getGcPausesMetrics());
}
case "memory": {
case SpringProcessConnectorService.MEMORY: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The metricName passed here are ("gcPauses", "heapMemory", "nonHeapMemory"). This is used to fetch the region-specific metric data separately.
(This is the result of my bad naming convention again ;-) )

@BoykoAlex I would like to discuss and get your feedback on how to improve this.

@BoykoAlex
Copy link
Contributor Author

@vudayani-vmw I switched it back to the names you had. I noticed that in you're switching the actual metric name. GC Pauses was missing the proper metric name.
Also fixed a few things around logging 404s - don't think we'd want to log 404's as it might be that metric or any other piece of live data hasn't been exposed by the app. Also fixed the case of {ownerId} in the URL which was attempted to be subbed by variable value by the REST Template. Furthermore, double encoding should be fixed as well.

@BoykoAlex
Copy link
Contributor Author

Closed with a411663

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants