Skip to content

Conversation

@Shivd131
Copy link
Contributor

@Shivd131 Shivd131 commented Jan 19, 2026

Description

This PR aims to instrument the RunreportsApiResource with the @Timed annotation to provide visibility into report generation performance.

Currently, there is no specific metric isolating the Run Reports API execution time from general HTTP traffic. This change introduces a dedicated metric, fineract.report.execution, to track the aggregate latency and throughput of the reporting subsystem.


Key Changes

  • Annotation-based Metrics
    Applied @Timed to the runReport method in RunreportsApiResource.

  • Infrastructure Configuration
    Added MetricsConfig to enable AOP support for the @Timed annotation by registering TimedAspect.

  • Clean Implementation
    Utilized standard Spring Boot and Micrometer patterns, avoiding any additional boilerplate.

This establishes the necessary performance baseline to monitor the reporting module’s load and response times.


Verification

Validated locally by generating report traffic and querying the Actuator metrics endpoint.

Request

curl -k -u mifos:password \
  -H "Fineract-Platform-TenantId: default" \
  -X GET "https://localhost:8443/fineract-provider/actuator/metrics/fineract.report.execution"
Response
{
  "name": "fineract.report.execution",
  "description": "Time taken to execute reports",
  "baseUnit": "seconds",
  "measurements": [
    {
      "statistic": "COUNT",
      "value": 1.0
    },
    {
      "statistic": "TOTAL_TIME",
      "value": 0.0114712
    },
    {
      "statistic": "MAX",
      "value": 0.0114712
    }
  ],
  "availableTags": [
    {
      "tag": "exception",
      "values": ["none"]
    },
    {
      "tag": "component",
      "values": ["reporting"]
    },
    {
      "tag": "method",
      "values": ["runReport"]
    },
    {
      "tag": "class",
      "values": [
        "org.apache.fineract.infrastructure.dataqueries.api.RunreportsApiResource"
      ]
    }
  ]
}

Checklist

Please make sure these boxes are checked before submitting your pull request — thanks!

  • Write the commit message as per our guidelines
  • Acknowledge that we will not review PRs that are not passing the build ("green") — it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
  • Create/update unit or integration tests for verifying the changes made.
  • Follow our coding conventions.
  • Add required Swagger annotations and update API documentation at
    fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm if applicable.
  • This PR must not be a "code dump".

Your assigned reviewer(s) will follow our
guidelines for code reviews.

Copy link
Contributor

@IOhacker IOhacker left a comment

Choose a reason for hiding this comment

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

Kidnly see my comment

Copy link
Contributor

@IOhacker IOhacker left a comment

Choose a reason for hiding this comment

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

.

@IOhacker
Copy link
Contributor

Squash and commit your changes please

@Shivd131 Shivd131 changed the title [FINERACT-2438] Add Micrometer Observability to Reporting API FINERACT-2438: Add Micrometer Observability to Reporting API Jan 20, 2026
@Shivd131 Shivd131 force-pushed the feature/report-metrics branch 2 times, most recently from e7c4794 to a2e3c06 Compare January 20, 2026 15:53
@Shivd131
Copy link
Contributor Author

@IOhacker I have squashed the changes into a single commit.
Thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont like the idea of injecting and doing manually the metric collection.

Please use "Spring (Boot 3 / Micrometer Observation): @observed" or "Classic Micrometer annotations: @timed" instead.

management.info.git.mode=FULL
management.endpoints.web.exposure.include=${FINERACT_MANAGEMENT_ENDPOINT_WEB_EXPOSURE_INCLUDE:health,info,prometheus}

management.endpoints.web.exposure.include=${FINERACT_MANAGEMENT_ENDPOINT_WEB_EXPOSURE_INCLUDE:health,info,metrics,prometheus}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please dont change this configuration. Metrics are not exposed by default for a reason!

management.endpoints.web.exposure.include=${FINERACT_MANAGEMENT_ENDPOINT_WEB_EXPOSURE_INCLUDE:health,info,prometheus}

management.endpoints.web.exposure.include=${FINERACT_MANAGEMENT_ENDPOINT_WEB_EXPOSURE_INCLUDE:health,info,metrics,prometheus}
management.endpoint.metrics.enabled=${FINERACT_MANAGEMENT_ENDPOINT_METRICS_ENABLED:false}
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think it is valid endpoint anymore, it was changed to be management.endpoints.web.exposure.include=metrics no?

Copy link
Contributor

@adamsaghy adamsaghy left a comment

Choose a reason for hiding this comment

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

Kindly see my concerns!

@Shivd131 Shivd131 force-pushed the feature/report-metrics branch from ec4fed4 to 263acb0 Compare January 25, 2026 04:13
@Shivd131
Copy link
Contributor Author

@adamsaghy Thanks for the detailed review.

I have refactored the PR to align with your feedback:

  • Reverted application.properties

  • Switched to @timed: Removed the manual Timer boilerplate from the resource file. I added a small MetricsConfig class to enable the TimedAspect bean so the annotation works correctly with Jersey.

  • Context on the original approach: The reason I initially used manual instrumentation was to capture dynamic tags like reportName and outputType. For my future projects, determining exactly which specific reports are resource-intensive is crucial for deciding what to offload to a microservice.

The current @timed implementation aggregates all reports into one metric, which hides that granularity, but I agree this is the cleaner, standard starting point for the codebase.

I am open to any suggestions/corrections if required

@Shivd131 Shivd131 force-pushed the feature/report-metrics branch from 0903594 to fdd4ca3 Compare January 25, 2026 06:12
@Shivd131
Copy link
Contributor Author

@IOhacker I think I made a mistake while copying the license header at the top of the code file earlier, which I suspect made the code fail the check. I’ve corrected it now, and the build check should pass.

@adamsaghy adamsaghy merged commit 55f764d into apache:develop Jan 26, 2026
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants