feat: apply REST performance time range filters

[OoreFasawe]

Pull Request

Description

Implement REST performance time-range filtering and query-driven chart filtering.

This PR builds on the merged REST query-string parsing work. It applies parsed query parameters to the performance and chart endpoints, adds bounded historical data storage, records live performance samples, and documents the resulting API behavior.

Example performance request:

GET /api/v1/strategies/primary_strategy/performance?start=100&end=300&limit=50

returns matching performance snapshots within the inclusive range.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Performance improvement
• Security enhancement
• Refactoring (no functional changes)

Areas Changed

• Core Engine (order book, execution)
• Trading Strategies
• Exchange Connectivity
• Security Infrastructure
• Performance/Benchmarks
• Documentation
• CI/CD
• Visualization / REST API
• Tests

Testing

• Unit tests pass locally
• Performance benchmarks run successfully
• Manual testing completed
• Security validation performed (if applicable)

Validation completed:

WebServerTests: passed
CTest: 100% tests passed
C++ pre-commit hooks: passed

Added tests for:

• Inclusive performance time-range filtering
• Performance result limits
• Maximum history-size enforcement
• Chart timestamp-range filtering
• Query-bearing chart route extraction
• Invalid and reversed performance ranges
• Invalid performance limits
• Empty performance ranges
• Latest snapshot object response shape
• Historical array response shape
• Strategy history cleanup after unregisterStrategy()

Performance Impact

• No performance impact
• Performance improvement (include benchmark results)
• Potential performance regression (justify and include mitigation)

Performance history is bounded by maxHistorySize, which prevents unbounded memory growth.

Performance range queries can also stop after reaching the requested limit.

Security Considerations

• No security implications
• Security enhancement
• Requires security review

Timestamp values are parsed with std::from_chars, and malformed or reversed ranges return HTTP 400 Bad Request. Unknown query parameters are ignored.

API Behavior

Performance Endpoint

GET /api/v1/strategies/{strategy_id}/performance

Supported parameters:

start=<timestamp>
end=<timestamp>
limit=<number>

Example:

GET /api/v1/strategies/primary_strategy/performance?start=100&end=300&limit=2

Requests without start, end, or limit preserve the existing response format and return the latest performance snapshot as an object.

Requests with any of these parameters return an array of matching historical snapshots.

start and end bounds are inclusive.

Chart Endpoint
GET /api/v1/strategies/{strategy_id}/charts/{metric}

Supported parameters:

range=<duration>
start=<timestamp>
end=<timestamp>

Supported range units include:

s  seconds
m  minutes
h  hours
d  days

Examples:
GET /api/v1/strategies/primary_strategy/charts/pnl?range=1h
GET /api/v1/strategies/primary_strategy/charts/pnl?start=100&end=300

Explicit start and end values take precedence over range.

Invalid or reversed chart ranges return HTTP 400 Bad Request.

Data Collection

The following APIs are available for populating retained data:

PerformanceCollector::recordPerformance(...)
PerformanceCollector::recordChartData(...)
VisualizationServer::recordPerformance(...)
VisualizationServer::recordChartData(...)

When visualization is enabled, the main application loop records live performance snapshots containing the timestamp, PnL, and position.

Chart history remains empty until chart-data producers call recordChartData().

Documentation

Updated:

• README.md
• docs/STRATEGY_PERFORMANCE_VISUALIZATION.md

The documentation describes supported query parameters, response shapes, range validation, and bounded history behavior.

Checklist

• Code follows project style guidelines
• Self-review of the code completed
• Code is commented, particularly in hard-to-understand areas
• Corresponding changes to documentation made
• Tests added that prove the fix is effective or that the feature works
• Any dependent changes have been merged and published

The dependent query-parsing changes have been published in the base branch. The base PR should be merged before this PR is retargeted to main.

Related Issues

• Closes Use query parameters in the performance REST endpoints #75
• Chart-data functionality is also related to Honor query parameters in handleGetChartData and implement getChartData #84.

Screenshots/Benchmark Results

Not applicable

Additional Notes

The implementation is organized into focused commits:

• feat: support REST performance result limits
• feat: route REST strategy chart requests
• feat: support REST chart time ranges
• test: cover REST performance query handling
• feat: record live performance history
• docs: document REST query filters

Existing behavior is preserved for requests without supported query parameters.

[coderabbitai]

Warning

Review limit reached

@OoreFasawe, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 4 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1fd045e7-ab30-4b4f-aad9-279510db91d3

📥 Commits

Reviewing files that changed from the base of the PR and between 8783b8f and 9139b0b.

📒 Files selected for processing (3)

• tests/unit/WebServerTests.cpp
• visualization/WebServer.cpp
• visualization/WebServer.h

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[chizy7]

Thanks for taking this one on. I went through the diff and also ran it against what #75 asks for, and the start/end handling on the performance endpoint is clean. CI is green on every job in the matrix, and bounding the history and trimming it again in setMaxHistorySize is a nice touch that I did not ask for but am glad to see. That said, there are a few things I want sorted before this can close the issue.

1. handleGetChartData was not touched

The issue covers both handlers, not just performance. handleGetChartData still opens with boost::ignore_unused(query) and still hardcodes the one hour range (3600000000000ULL). I would like either a range or start/end parameter applied there too, or if you would rather keep this PR focused on the performance endpoint, say so in the description. I opened #84 to track the chart data side (it also covers filling in the getChartData stub), so scoping this PR down is fine with me as long as it is a deliberate choice and not something that slipped through.

2. Nothing actually tests the handler

The new tests cover PerformanceCollector::getPerformanceHistory and the eviction, which is good, but nothing calls handleGetPerformance with a query string. So the 400 path for a malformed or reversed range, the inclusive bounds parsing and the array versus object response shape are all unverified right now. When I reviewed #72 we dropped RoutesPerformanceRequestsWithQueryStrings because it needed private access to handleRequest. If you add a small public entry point for handler dispatch (or a friend test fixture), that would cover this PR and bring back the routing coverage at the same time. A few more collector cases would help as well: start equal to the first timestamp, a range that matches nothing, and unregisterStrategy clearing the history (which you added in this PR but did not test).

3. Nobody calls recordPerformance

You called this out yourself in the description, and I confirmed it, no producer in the codebase calls VisualizationServer::recordPerformance. Which means that in a real run, a request with start and end will always come back with an empty array. Either wire it up from the strategy loop in main.cpp as part of this PR, or let us file a tracked follow up issue and link it here so it does not get lost.

4. The response shape changes depending on the query

Without parameters data is an object, with parameters it is an array. That is workable but it has to be written down in the REST API docs (README and docs/), and the PR currently has the documentation box unchecked. The other option is to keep the object as it is and only add a history array when a range is supplied, which avoids switching the shape entirely. Your call, just document whichever one you pick.

5. PR housekeeping

The trailing "- #1" in the title is the PR number from your fork, and since semantic-release builds the release notes from titles it will end up there and confuse people. Something like "feat(api): apply start/end time range to performance endpoint" works. Please also reference the issue in the body, Refs #75 for now, or Closes #75 once points 1 and 2 are done. And the PR is opened from your fork's main branch, which works, but any unrelated push to your main will land in here, so a feature branch is safer next time.

Minor things

In setMaxHistorySize the structured binding plus boost::ignore_unused(strategyId) can just be for (auto& entry : m_performanceHistory) using entry.second. Also, returning 400 on a reversed range instead of falling back like the issue suggested is the better choice in my opinion, so no change needed there, I am only noting it so it is on record as intentional.

Ping me when these are in and I will re-review quickly.

[OoreFasawe]

1. handleGetChartData() was not touched

Addressed.

handleGetChartData() now:

• Parses query parameters.
• Supports range, such as 30m, 1h, and 1d.
• Supports explicit start and end timestamp bounds.
• Gives explicit start and end precedence over range.
• Returns 400 Bad Request for invalid or reversed ranges.
• Preserves the one-hour default when no range is supplied.
• Uses bounded chart-history storage through getChartDataInRange().

The chart endpoint is now routed through:
GET /api/v1/strategies/{strategy_id}/charts/{metric}

2. Nothing tests the handler

Addressed.

A public RestAPIServer::handleRequest() entry point is now available for request-dispatch testing without adding test-specific friendship to the production header.

Added tests covering:

• Query-bearing performance requests
• Inclusive start and end bounds
• Invalid ranges
• Reversed ranges
• Invalid limit values
• Empty matching ranges
• Latest snapshot response shape
• Historical array response shape
• Strategy history cleanup after unregisterStrategy()

3. Nobody calls recordPerformance()

Addressed.

The main application loop now records live performance data when visualization is enabled:

• Timestamp
• PnL
• Position

The data is recorded through:
VisualizationServer::recordPerformance(...)

This means live performance history can now be populated for REST range queries.

Chart data has a corresponding API:
VisualizationServer::recordChartData(...)

Chart producers still need to call that API when chart samples are available.

4. Response shape changes depending on the query

Addressed and documented.

Existing requests without query parameters continue returning the latest snapshot as an object:

{
  "success": true,
  "data": {
    "pnl": 1.5
  }
}

Requests using start, end, or limit return an array of matching historical snapshots:

{
  "success": true,
  "data": [
    {
      "timestamp": 100,
      "pnl": 1.5
    }
  ]
}

This behavior is now documented in:

``README.md STRATEGY_PERFORMANCE_VISUALIZATION.md```

5. PR housekeeping

Fixed. The PR title no longer contains the incorrect #1 suffix.

[chizy7]

Thanks for the quick turnaround on this. Before I get into the code I have to flag something, because I almost reviewed the wrong thing. The changes you describe in your comment are not in this PR. The PR is still pointing at your fork's main branch, which is sitting at 9139b0b with the same four commits and the same +156/-10 diff I reviewed on the 20th. The new work is on your feat/performance-query-filters branch (0e9e79e, six commits ahead), and it was never merged into your main. So GitHub is showing me the old diff and CI has not run on any of the new code. Please either retarget this PR to feat/performance-query-filters (that is the cleaner option, and it also solves the "PR from main" thing I mentioned last time) or push that branch into your main. Until one of those happens there is nothing new here for me to approve.

That said, I pulled the branch and went through it anyway so you have feedback ready. I also built it locally in Debug and ran web_server_tests, 17 out of 17 passed, so the code itself is in good shape. Since this PR is meant to close both #75 and #84, I checked it against both. Against #75 it now covers everything the issue asks for, and the public handleRequest with the RestHandler fixture is exactly the seam I was hoping for. Nice work on that. Against #84 it is about two thirds of the way there: the charts route, the range and start/end parsing, the 400s and the one hour default are all done, but the part of #84 that asks for getChartData to actually return data from the retained history is not (more on that in point 5), and the chart handler has no request level tests yet (point 4). Please also add "Closes #75" and "Closes #84" to the PR body so they get closed automatically when this merges.

A few things for the next round, none of them big.

1. Sampling rate in main.cpp

recordPerformance is called on every iteration of the main loop and the loop sleeps 100ms, so that is roughly ten samples a second. With maxHistorySize at 10000 the history only holds about 17 minutes, which makes range=1d on the chart endpoint meaningless in practice. There is already a vizConfig.dataCollectionIntervalMs (1000ms) that gets passed into startCollection, and startCollection does nothing with it. Rate limit the recording with that interval, or just record inside the existing five second stats block, either works.

2. limit returns the oldest N, not the newest N

getPerformanceHistory scans oldest to newest and stops after the first limit matches. Someone calling ?limit=50 with no start almost certainly wants the most recent 50, not the first 50 in the buffer. Either scan from the back or spell the semantics out in the docs. Related, limit=0 silently means unlimited and the docs only say "limits the number of returned snapshots", so that should be written down too (or rejected).

3. Wrong error message

An invalid limit comes back with "Invalid performance time range". It should say limit.

4. The chart handler has no request level tests

The RestHandler tests only exercise /performance. Please add the chart side too: ?range=30m happy path, ?range=1x and ?range=m returning 400, start greater than end returning 400, and start/end winning over range when both are supplied. parseRange (the size check, the unit switch, the overflow guard) is exactly the kind of code I want tests around, and once point 5 is done, a test that records a few performance samples and reads them back through /charts/pnl?range=1h would cover the whole path end to end.

5. Chart data still has no producer, and #84 asks for it to be built on the performance history

Same gap as last time, just moved one level down. recordChartData exists but nothing calls it, so /charts/{metric} returns an empty array in a live run. #84 specifically asks for getChartData to be implemented on top of the bounded history that this PR already fills through recordPerformance, so the endpoint returns real points for pnl, position, sharpe_ratio, max_drawdown, win_rate, total_trades, ml_accuracy and prediction_time. PerformanceData already carries all of those, so getChartDataInRange can map the metric name to the matching field and read from m_performanceHistory. That also means you can delete m_chartHistory, chartHistoryKey, recordChartData, the '\0' prefix scan in unregisterStrategy and the second trim loop in setMaxHistorySize, which is less code than what is there now. Right now an unknown metric silently returns an empty array; #84 asks you to pick between that and a 400 and document it, and I would go with 400 so a typo in the metric name does not look like "no data".

Minor things, take or leave:

• handleGetChartData computes startTime as endTime minus one hour without the underflow guard that getChartData has. Harmless today, just inconsistent.
• The parseTimestamp lambda and the 400 response boilerplate are duplicated across both handlers. A small badRequest(msg) helper would remove about 25 lines.
• The two for (auto& [k, history] : ...) loops with boost::ignore_unused(k) in setMaxHistorySize can just iterate entry.second.
• Chart JSON now includes label and color, which will be empty strings from most callers. Fine, but mention it in the docs if it stays.

Good catch updating STRATEGY_PERFORMANCE_VISUALIZATION.md from start_time/end_time/timeframe to the new names, I had not noticed that doc was out of date.

Retarget the PR first so CI and CodeRabbit can see the real diff, then ping me once the points above are in and I will do another pass. Point 5 is the only one with real work in it, the rest are small. I think this is one round away.