Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,18 @@
<artifactId>jsoup</artifactId>
<version>1.18.1</version>
</dependency>
<dependency>
<groupId>io.micrometer</groupId>
<artifactId>micrometer-registry-prometheus</artifactId>
</dependency>
<dependency>
<groupId>org.aspectj</groupId>
<artifactId>aspectjweaver</artifactId>
</dependency>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-actuator</artifactId>
</dependency>
</dependencies>
<build>
<plugins>
Expand Down
21 changes: 14 additions & 7 deletions core/src/main/java/greencity/config/SecurityConfig.java
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
package greencity.config;

import greencity.metrics.ActiveUsersInMemoryMetrics;
import greencity.metrics.RequestCountMetrics;
import greencity.security.filters.AccessTokenAuthenticationFilter;
import greencity.security.filters.XSSFilter;
import greencity.security.jwt.JwtTool;
import greencity.security.providers.JwtAuthenticationProvider;
import greencity.service.UserService;
import java.util.Arrays;
import java.util.List;
import io.micrometer.core.instrument.MeterRegistry;
import lombok.RequiredArgsConstructor;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.boot.actuate.autoconfigure.security.servlet.EndpointRequest;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.http.HttpMethod;
Expand All @@ -26,10 +28,9 @@
import org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter;
import org.springframework.security.web.util.matcher.AntPathRequestMatcher;
import org.springframework.web.cors.CorsConfiguration;
import static greencity.constant.AppConstant.ADMIN;
import static greencity.constant.AppConstant.USER;
import static greencity.constant.AppConstant.MODERATOR;
import static greencity.constant.AppConstant.UBS_EMPLOYEE;
import java.util.Arrays;
import java.util.List;
import static greencity.constant.AppConstant.*;
import static jakarta.servlet.http.HttpServletResponse.SC_FORBIDDEN;
import static jakarta.servlet.http.HttpServletResponse.SC_UNAUTHORIZED;
import static org.springframework.security.config.http.SessionCreationPolicy.STATELESS;
Expand Down Expand Up @@ -88,6 +89,8 @@ public class SecurityConfig {
private final JwtTool jwtTool;
private final UserService userService;
private final AuthenticationConfiguration authenticationConfiguration;
private final MeterRegistry meterRegistry;
private final RequestCountMetrics requestCountMetrics;

@Value("${spring.messaging.stomp.websocket.allowed-origins}")
private String[] allowedOrigins;
Expand Down Expand Up @@ -125,6 +128,8 @@ public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Excepti
UsernamePasswordAuthenticationFilter.class)
.addFilterBefore(new XSSFilter(),
UsernamePasswordAuthenticationFilter.class)
.addFilterAfter(new ActiveUsersInMemoryMetrics(meterRegistry), AccessTokenAuthenticationFilter.class)
.addFilterBefore(requestCountMetrics, AccessTokenAuthenticationFilter.class)
.exceptionHandling(exception -> exception.authenticationEntryPoint((req, resp, exc) -> resp
.sendError(SC_UNAUTHORIZED, "Authorize first."))
.accessDeniedHandler((req, resp, exc) -> resp.sendError(SC_FORBIDDEN, "You don't have authorities.")))
Expand All @@ -139,8 +144,10 @@ public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Excepti
"/swagger-ui.html",
"/swagger-ui/**",
"/swagger-resources/**",
"/webjars/**")
"/webjars/**",
"/metrics/recordLogin")
Comment on lines +147 to +148
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security consideration: Publicly accessible metrics endpoint

The /metrics/recordLogin endpoint is publicly accessible, which might allow malicious users to submit fake login events and skew your metrics. Consider restricting this endpoint to authenticated users or adding rate limiting.

- "/webjars/**",
- "/metrics/recordLogin")
+ "/webjars/**")
  .permitAll()
+ .requestMatchers("/metrics/recordLogin")
+ .hasAnyRole(USER, ADMIN, MODERATOR, UBS_EMPLOYEE)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"/webjars/**",
"/metrics/recordLogin")
// before:
.requestMatchers(
"/webjars/**",
"/metrics/recordLogin")
.permitAll()
// after:
.requestMatchers("/webjars/**")
.permitAll()
.requestMatchers("/metrics/recordLogin")
.hasAnyRole(USER, ADMIN, MODERATOR, UBS_EMPLOYEE)

.permitAll()
.requestMatchers(EndpointRequest.toAnyEndpoint()).permitAll()
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security concern: Unrestricted access to actuator endpoints

Allowing unrestricted access to all actuator endpoints (.requestMatchers(EndpointRequest.toAnyEndpoint()).permitAll()) can expose sensitive information and operations. Consider restricting access to authenticated users or specific roles.

- .requestMatchers(EndpointRequest.toAnyEndpoint()).permitAll()
+ // Provide limited access to health and info endpoints
+ .requestMatchers(EndpointRequest.to("health", "info")).permitAll()
+ // Restrict other actuator endpoints to admin role
+ .requestMatchers(EndpointRequest.toAnyEndpoint()).hasRole(ADMIN)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
.requestMatchers(EndpointRequest.toAnyEndpoint()).permitAll()
.authorizeHttpRequests()
// Provide limited access to health and info endpoints
.requestMatchers(EndpointRequest.to("health", "info")).permitAll()
// Restrict other actuator endpoints to admin role
.requestMatchers(EndpointRequest.toAnyEndpoint()).hasRole(ADMIN)
.anyRequest().authenticated()

.requestMatchers("/css/**", "/img/**").permitAll()
.requestMatchers(HttpMethod.GET,
FACT_OF_THE_DAY + RANDOM,
Expand Down
23 changes: 23 additions & 0 deletions core/src/main/java/greencity/controller/MetricsController.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package greencity.controller;

import greencity.dto.metric.LoginEventDto;
import greencity.metrics.ActiveUsersInMemoryMetrics;
import lombok.RequiredArgsConstructor;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;

@RestController
@RequestMapping("/metrics")
@RequiredArgsConstructor
public class MetricsController {
private final ActiveUsersInMemoryMetrics activeUsersInMemoryMetrics;

@PostMapping("/recordLogin")
public ResponseEntity<Void> recordLogin(@RequestBody LoginEventDto loginEventDto) {
activeUsersInMemoryMetrics.recordLogin(loginEventDto.getEmail(), loginEventDto.getLoginTime());
return ResponseEntity.ok().build();
}
}
Comment on lines +12 to +23
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation, exception handling, and API documentation.

The controller implementation is clean and focused, but could benefit from several improvements:

  1. Add input validation to ensure the LoginEventDto contains valid data
  2. Implement exception handling to gracefully manage potential errors
  3. Add API documentation for better visibility and understanding
  4. Consider security implications of this endpoint
+import javax.validation.Valid;
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;

 @RestController
 @RequestMapping("/metrics")
 @RequiredArgsConstructor
+@Api(tags = "Metrics", description = "Endpoints for application metrics")
 public class MetricsController {
     private final ActiveUsersInMemoryMetrics activeUsersInMemoryMetrics;

+    @ApiOperation(value = "Record user login event", notes = "Captures login events for metrics tracking")
+    @ApiResponses(value = {
+        @ApiResponse(code = 200, message = "Login event successfully recorded"),
+        @ApiResponse(code = 400, message = "Invalid input data"),
+        @ApiResponse(code = 500, message = "Internal server error")
+    })
     @PostMapping("/recordLogin")
-    public ResponseEntity<Void> recordLogin(@RequestBody LoginEventDto loginEventDto) {
-        activeUsersInMemoryMetrics.recordLogin(loginEventDto.getEmail(), loginEventDto.getLoginTime());
-        return ResponseEntity.ok().build();
+    public ResponseEntity<Void> recordLogin(@Valid @RequestBody LoginEventDto loginEventDto) {
+        try {
+            activeUsersInMemoryMetrics.recordLogin(loginEventDto.getEmail(), loginEventDto.getLoginTime());
+            return ResponseEntity.ok().build();
+        } catch (Exception e) {
+            // Log the exception
+            return ResponseEntity.internalServerError().build();
+        }
     }
 }

13 changes: 13 additions & 0 deletions core/src/main/java/greencity/dto/metric/LoginEventDto.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package greencity.dto.metric;

import lombok.AllArgsConstructor;
import lombok.Data;
import lombok.NoArgsConstructor;

@Data
@AllArgsConstructor
@NoArgsConstructor
public class LoginEventDto {
private String email;
private Long loginTime;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
package greencity.metrics;

import io.micrometer.core.instrument.Gauge;
import io.micrometer.core.instrument.MeterRegistry;
import jakarta.servlet.FilterChain;
import jakarta.servlet.ServletException;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.stereotype.Component;
import org.springframework.web.filter.OncePerRequestFilter;
import java.io.IOException;
import java.time.Instant;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;

@Component
public class ActiveUsersInMemoryMetrics extends OncePerRequestFilter {
private static final Logger logger = LoggerFactory.getLogger(ActiveUsersInMemoryMetrics.class);
private final MeterRegistry meterRegistry;
private final Map<String, Instant> activeUsers = new ConcurrentHashMap<>();
private final Map<String, List<Instant>> userLogins = new ConcurrentHashMap<>();
private static final long INACTIVITY_TIMEOUT_MINUTES = 60;
private static final long INACTIVITY_TIMEOUT_SECONDS = INACTIVITY_TIMEOUT_MINUTES * 60;
private static final long LOGIN_WINDOW_HOURS = 3;
private static final long LOGIN_WINDOW_SECONDS = LOGIN_WINDOW_HOURS * 3600;
Comment on lines +26 to +31
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider limiting the maximum capacity of tracking maps

The maps for tracking users have no size limits, which could lead to memory issues with high user counts. Consider adding maximum capacity constraints.

You might want to:

  1. Add configuration properties for maximum capacity
  2. Implement an eviction strategy when limits are reached (e.g., LRU - Least Recently Used)
  3. Add monitoring for the current size vs. capacity


public ActiveUsersInMemoryMetrics(MeterRegistry meterRegistry) {
this.meterRegistry = meterRegistry;

Gauge.builder("app_active_users", this::getActiveUsersCount)
.description("Number of active users on the site")
.baseUnit("users")
.register(meterRegistry);

Gauge.builder("app_user_logins_per_3h", this::getUserLoginsCount)
.description("Number of unique user logins in the last 3 hours")
.baseUnit("users")
.register(meterRegistry);
}

@Override
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
throws ServletException, IOException {
Authentication auth = SecurityContextHolder.getContext().getAuthentication();
if (auth != null && auth.isAuthenticated() && !"anonymousUser".equals(auth.getName())) {
String username = auth.getName();
Instant now = Instant.now();
activeUsers.put(username, now);
}

removeInactiveUsers();
filterChain.doFilter(request, response);
Comment on lines +57 to +58
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider optimizing removeInactiveUsers() call frequency

Calling removeInactiveUsers() on every request could impact performance, especially with high traffic. Consider implementing a time-based throttling mechanism to reduce cleanup frequency.

@Override
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
    throws ServletException, IOException {
    Authentication auth = SecurityContextHolder.getContext().getAuthentication();
    if (auth != null && auth.isAuthenticated() && !"anonymousUser".equals(auth.getName())) {
        String username = auth.getName();
        Instant now = Instant.now();
        activeUsers.put(username, now);
    }

-   removeInactiveUsers();
+   // Only perform cleanup periodically to improve performance
+   if (shouldPerformCleanup()) {
+       removeInactiveUsers();
+   }
    filterChain.doFilter(request, response);
}

+ private final AtomicLong lastCleanupTime = new AtomicLong(0);
+ private static final long CLEANUP_INTERVAL_MS = 60_000; // Clean up once per minute
+ 
+ private boolean shouldPerformCleanup() {
+     long currentTime = System.currentTimeMillis();
+     long lastCleanup = lastCleanupTime.get();
+     
+     if (currentTime - lastCleanup > CLEANUP_INTERVAL_MS) {
+         // Try to update the last cleanup time, but only if another thread hasn't already done so
+         return lastCleanupTime.compareAndSet(lastCleanup, currentTime);
+     }
+     return false;
+ }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
removeInactiveUsers();
filterChain.doFilter(request, response);
@Override
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
throws ServletException, IOException {
Authentication auth = SecurityContextHolder.getContext().getAuthentication();
if (auth != null && auth.isAuthenticated() && !"anonymousUser".equals(auth.getName())) {
String username = auth.getName();
Instant now = Instant.now();
activeUsers.put(username, now);
}
// Only perform cleanup periodically to improve performance
if (shouldPerformCleanup()) {
removeInactiveUsers();
}
filterChain.doFilter(request, response);
}
// Throttle inactivity cleanup to once per minute
private final AtomicLong lastCleanupTime = new AtomicLong(0);
private static final long CLEANUP_INTERVAL_MS = 60_000; // 1 minute
private boolean shouldPerformCleanup() {
long currentTime = System.currentTimeMillis();
long lastCleanup = lastCleanupTime.get();
if (currentTime - lastCleanup > CLEANUP_INTERVAL_MS) {
// Only one thread should actually perform the cleanup
return lastCleanupTime.compareAndSet(lastCleanup, currentTime);
}
return false;
}

}

public void recordLogin(String email, Long loginTime) {
Instant signInInstant = Instant.ofEpochMilli(loginTime);
userLogins.computeIfAbsent(email, k -> new ArrayList<>()).add(signInInstant);
logger.debug("New login recorded for user: {} at time: {}", email, signInInstant);

removeInactiveUsers();
}

private void removeInactiveUsers() {
Instant now = Instant.now();
activeUsers.entrySet()
.removeIf(entry -> now.getEpochSecond() - entry.getValue().getEpochSecond() > INACTIVITY_TIMEOUT_SECONDS);

userLogins.forEach((key, logins) -> logins
.removeIf(login -> now.getEpochSecond() - login.getEpochSecond() > LOGIN_WINDOW_SECONDS));
userLogins.entrySet().removeIf(entry -> entry.getValue().isEmpty());
}

private int getActiveUsersCount() {
return activeUsers.size();
}

private int getUserLoginsCount() {
return userLogins.values().stream().mapToInt(List::size).sum();
}
}
34 changes: 34 additions & 0 deletions core/src/main/java/greencity/metrics/CPUMetrics.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package greencity.metrics;

import com.sun.management.OperatingSystemMXBean;
import io.micrometer.core.instrument.Gauge;
import io.micrometer.core.instrument.MeterRegistry;
import org.springframework.scheduling.annotation.Scheduled;
import org.springframework.stereotype.Component;
import java.lang.management.ManagementFactory;

@Component
public class CPUMetrics {
private final MeterRegistry meterRegistry;
private final OperatingSystemMXBean osBean;

public CPUMetrics(MeterRegistry meterRegistry) {
this.meterRegistry = meterRegistry;
this.osBean = (OperatingSystemMXBean) ManagementFactory.getOperatingSystemMXBean();

Gauge.builder("app_cpu_usage_percent", this, CPUMetrics::getCpuUsage)
.description("CPU usage of the application in percent")
.baseUnit("percent")
.register(meterRegistry);
}
Comment on lines +15 to +23
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle unavailable com.sun.management.OperatingSystemMXBean implementation

The current implementation assumes that ManagementFactory.getOperatingSystemMXBean() will always return an instance of com.sun.management.OperatingSystemMXBean, which might not be true in all JVM implementations. Add a safety check to handle cases where the extended API is not available.

public CPUMetrics(MeterRegistry meterRegistry) {
    this.meterRegistry = meterRegistry;
-   this.osBean = (OperatingSystemMXBean) ManagementFactory.getOperatingSystemMXBean();
+   java.lang.management.OperatingSystemMXBean standardBean = ManagementFactory.getOperatingSystemMXBean();
+   if (standardBean instanceof OperatingSystemMXBean) {
+       this.osBean = (OperatingSystemMXBean) standardBean;
+   } else {
+       throw new IllegalStateException("OperatingSystemMXBean from com.sun.management package is not available in this JVM");
+   }

    Gauge.builder("app_cpu_usage_percent", this, CPUMetrics::getCpuUsage)
        .description("CPU usage of the application in percent")
        .baseUnit("percent")
        .register(meterRegistry);
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public CPUMetrics(MeterRegistry meterRegistry) {
this.meterRegistry = meterRegistry;
this.osBean = (OperatingSystemMXBean) ManagementFactory.getOperatingSystemMXBean();
Gauge.builder("app_cpu_usage_percent", this, CPUMetrics::getCpuUsage)
.description("CPU usage of the application in percent")
.baseUnit("percent")
.register(meterRegistry);
}
public CPUMetrics(MeterRegistry meterRegistry) {
this.meterRegistry = meterRegistry;
java.lang.management.OperatingSystemMXBean standardBean = ManagementFactory.getOperatingSystemMXBean();
if (standardBean instanceof com.sun.management.OperatingSystemMXBean) {
this.osBean = (com.sun.management.OperatingSystemMXBean) standardBean;
} else {
throw new IllegalStateException(
"OperatingSystemMXBean from com.sun.management package is not available in this JVM"
);
}
Gauge.builder("app_cpu_usage_percent", this, CPUMetrics::getCpuUsage)
.description("CPU usage of the application in percent")
.baseUnit("percent")
.register(meterRegistry);
}


@Scheduled(fixedRate = 60000)
public void updateCPUMetrics() {
meterRegistry.gauge("app_cpu_usage_percent", this, CPUMetrics::getCpuUsage);
}
Comment on lines +25 to +28
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove redundant gauge registration in the scheduled method

Similar to MemoryMetrics, the gauge is already registered in the constructor, so re-registering it in the scheduled method is unnecessary and could create duplicate metrics or memory leaks. Micrometer gauges automatically update when their function is called.

@Scheduled(fixedRate = 60000)
public void updateCPUMetrics() {
-   meterRegistry.gauge("app_cpu_usage_percent", this, CPUMetrics::getCpuUsage);
}

You can either remove the method entirely or leave it empty if it might be needed for future implementations.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Scheduled(fixedRate = 60000)
public void updateCPUMetrics() {
meterRegistry.gauge("app_cpu_usage_percent", this, CPUMetrics::getCpuUsage);
}
@Scheduled(fixedRate = 60000)
public void updateCPUMetrics() {
}


private double getCpuUsage() {
double cpuLoad = osBean.getProcessCpuLoad() * 100;
return cpuLoad >= 0 ? cpuLoad : 0;
}
}
125 changes: 125 additions & 0 deletions core/src/main/java/greencity/metrics/GarbageCollectionMetrics.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
package greencity.metrics;

import io.micrometer.core.instrument.Gauge;
import io.micrometer.core.instrument.MeterRegistry;
import org.springframework.stereotype.Component;
import java.lang.management.GarbageCollectorMXBean;
import java.lang.management.ManagementFactory;
import java.time.Instant;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentLinkedQueue;

@Component
public class GarbageCollectionMetrics {
private static final long TIME_WINDOW_SECONDS = 3600;
private final MeterRegistry meterRegistry;
private final List<GarbageCollectorMXBean> gcBeans;
private final Map<GarbageCollectorMXBean, ConcurrentLinkedQueue<GCTimeRecord>> gcTimeHistory =
new ConcurrentHashMap<>();
private final Map<GarbageCollectorMXBean, ConcurrentLinkedQueue<GCCountRecord>> gcCountHistory =
new ConcurrentHashMap<>();

public GarbageCollectionMetrics(MeterRegistry meterRegistry) {
this.meterRegistry = meterRegistry;
this.gcBeans = ManagementFactory.getGarbageCollectorMXBeans();
for (GarbageCollectorMXBean gcBean : gcBeans) {
gcTimeHistory.put(gcBean, new ConcurrentLinkedQueue<>());
gcCountHistory.put(gcBean, new ConcurrentLinkedQueue<>());
}
registerGauges();
}

private void registerGauges() {
for (GarbageCollectorMXBean gcBean : gcBeans) {
Gauge.builder("app_gc_time_ms_per_hour", this, metrics -> getGCTimePerHour(gcBean))
.description("Time spent in garbage collection per hour in milliseconds")
.baseUnit("milliseconds")
.tag("collector", gcBean.getName())
.register(meterRegistry);

Gauge.builder("app_gc_count_per_hour", this, metrics -> getGCCountPerHour(gcBean))
.description("Number of garbage collection cycles per hour")
.baseUnit("cycles")
.tag("collector", gcBean.getName())
.register(meterRegistry);
}
}

private double getGCTimePerHour(GarbageCollectorMXBean gcBean) {
return getGCTimePerHour(gcBean, Instant.now());
}

double getGCTimePerHour(GarbageCollectorMXBean gcBean, Instant now) {
ConcurrentLinkedQueue<GCTimeRecord> history = gcTimeHistory.get(gcBean);
long currentGCTime = gcBean.getCollectionTime();

history.add(new GCTimeRecord(now, currentGCTime));
cleanupOldGCTimeRecords(gcBean, now);

if (history.size() < 2) {
return 0.0;
}

GCTimeRecord oldest = history.peek();
long timeDiff = currentGCTime - oldest.gcTime;
return timeDiff >= 0 ? timeDiff : 0.0;
}
Comment on lines +54 to +68
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Reported “per-hour” GC time is not actually normalised to 1 hour

getGCTimePerHour returns the raw difference between the newest and oldest collectionTime.
If the gauge is scraped every 2 minutes, the oldest record might be only 2 minutes old, so the diff reflects 2 minutes of GC time but is exposed as “per hour”. The value therefore fluctuates with the scrape interval rather than true hourly GC time.

A quick fix is to normalise the delta by the elapsed seconds:

 long timeDiff = currentGCTime - oldest.gcTime;
-long elapsed = Duration.between(oldest.timestamp, now).getSeconds();
-return timeDiff >= 0 ? timeDiff : 0.0;
+long elapsed = Duration.between(oldest.timestamp, now).getSeconds();
+if (elapsed == 0 || timeDiff < 0) {
+    return 0.0;
+}
+return ((double) timeDiff) * 3600 / elapsed; // scale to one hour

The same adjustment is needed in getGCCountPerHour. Without this correction the metric is misleading and hard to alert on.


private double getGCCountPerHour(GarbageCollectorMXBean gcBean) {
return getGCCountPerHour(gcBean, Instant.now());
}

double getGCCountPerHour(GarbageCollectorMXBean gcBean, Instant now) {
ConcurrentLinkedQueue<GCCountRecord> history = gcCountHistory.get(gcBean);
long currentGCCount = gcBean.getCollectionCount();

history.add(new GCCountRecord(now, currentGCCount));
cleanupOldGCCountRecords(gcBean, now);

if (history.size() < 2) {
return 0.0;
}

GCCountRecord oldest = history.peek();
long countDiff = currentGCCount - oldest.gcCount;
return countDiff >= 0 ? countDiff : 0.0;
}
Comment on lines +74 to +88
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Apply the same normalisation logic to GC count metric

For symmetry and accuracy, update getGCCountPerHour exactly as suggested for getGCTimePerHour, scaling the countDiff by 3600 / elapsedSeconds. This maintains consistent semantics across both GC metrics.


void cleanupOldGCTimeRecords(GarbageCollectorMXBean gcBean, Instant now) {
ConcurrentLinkedQueue<GCTimeRecord> history = gcTimeHistory.get(gcBean);
Instant threshold = now.minusSeconds(TIME_WINDOW_SECONDS);
while (!history.isEmpty() && history.peek().timestamp.isBefore(threshold)) {
history.poll();
}
}

void cleanupOldGCCountRecords(GarbageCollectorMXBean gcBean, Instant now) {
ConcurrentLinkedQueue<GCCountRecord> history = gcCountHistory.get(gcBean);
Instant threshold = now.minusSeconds(TIME_WINDOW_SECONDS);
while (!history.isEmpty() && history.peek().timestamp.isBefore(threshold)) {
history.poll();
}
}

private static class GCTimeRecord {
final Instant timestamp;
final long gcTime;

GCTimeRecord(Instant timestamp, long gcTime) {
this.timestamp = timestamp;
this.gcTime = gcTime;
}
}

private static class GCCountRecord {
final Instant timestamp;
final long gcCount;

GCCountRecord(Instant timestamp, long gcCount) {
this.timestamp = timestamp;
this.gcCount = gcCount;
}
}
}
Loading