EMFree data source should regard specified time range

[YaSuenag]

Pull Request

Issue Number: N/A

Summary

Make ElectricityMapsFree data source aware of specifying time range.
Currently WebAPI would return data even if it is out of time range which is specified in below:

Attempt to retrieve data to 2023-09-03T00:00:00Z from 2023-09-02T00:00:00Z

$ curl -s 'http://localhost:8082/emissions/bylocation?location=japaneast&startTime=2023-09-02T00%3A00%3A00Z&endTime=2023-09-03T00%3A00%3A00Z' | jq
[
  {
    "location": "japaneast",
    "time": "2023-09-04T00:00:00+00:00",  <- out of range!
    "rating": 530,
    "duration": "00:00:00"
  }
]

It should be returned empty (HTTP 204 or empty JSON array) or an error in this case.

Changes

• src/CarbonAware.DataSources/CarbonAware.DataSources.ElectricityMapsFree/src/ElectricityMapsFreeDataSource.cs
  • Key point of this PR. CO2Signal API does not have any parameters for time range, so EMFree data source filters before returning data when time range is specified.
• src/CarbonAware.CLI/test/integrationTests/Commands/Emissions/EmissionsCommandTests.cs
  • Only one test data would be generated in EMFree mock datasource, and it has emission data time equivalent with start time. Thus we need to cover it in specified time range.
  • In other case (WattTime, EM, JSON), multiple test data would be generated each 5 minutes. So we need to tweak time range within 5 minutes because number of response data would be checked by subsequent assertion.
• src/CarbonAware.CLI/test/integrationTests/Commands/EmissionsForecasts/EmissionsForecastsCommandTests.cs
  • This test would fail in some timezone (Japan at least). So add Z to time string because time is treated as UTC. (Let me know if this change should be happened in other issue)
• src/CarbonAware.DataSources/CarbonAware.DataSources.ElectricityMapsFree/test/ElectricityMapsFreeDataSourceTests.cs
  • Run tests both with/without time range

Checklist

• Local Tests Passing?
• CICD and Pipeline Tests Passing?
• Added any new Tests?
• Documentation Updates Made?
• Are there any API Changes? If yes, please describe below.
• This is not a breaking change. If it is, please describe it below.

Are there API Changes?

No

Is this a breaking change?

EMFree data source would not return any data when it does not exist within specified time range. But it is expected behavior I think.

[codecov-commenter]

Codecov Report

Merging #389 (1695a63) into dev (b3b3189) will increase coverage by 0.46%.
Report is 18 commits behind head on dev.
The diff coverage is 0.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##              dev     #389      +/-   ##
==========================================
+ Coverage   74.21%   74.67%   +0.46%     
==========================================
  Files          77       77              
  Lines        2637     2646       +9     
  Branches      266      268       +2     
==========================================
+ Hits         1957     1976      +19     
+ Misses        598      589       -9     
+ Partials       82       81       -1     

Files Changed	Coverage Δ
...icityMapsFree/src/ElectricityMapsFreeDataSource.cs	0.00% <0.00%> (ø)

... and 3 files with indirect coverage changes

[Willmish]

this all looks good, left a single comment to review on tomorrows meeting - otherwise happy to merge.

[Willmish]

Should we check below regardless of whether periodStartTime < periodEndTime?

This does catch any cases where the emissionsDateTime is outside of the range periodStartTime and periodEndTime if both are specified, but e.g. if only periodStartTime is specified for a time thats in the future, then it will still return the emission at current time.

Again, we might want to disregard this as this endpoint is only for current/historical data and no one should use dates for forecasting here, but maybe we should not return any data in that case as

[YaSuenag]

Agree, so is it better to throw exception (e.g. ArgumentOutOfRangeException ) instead of returning empty list in that case? I think it is an illegal case if GetCarbonIntensityAsync() receives future datetime, so I think it is appropriate to throw an exception.

[YaSuenag]

I think other datasources (WattTimeDataSource, ElectricityMapsDataSource) have same problems. If it is proper to throw exception, it is better to handle another issue/PR to fix all of datasources.

[Willmish]

Below is for WattTime (I requested 1 hour into the future, on the emissions/bylocation endpoint), so it is returning 204 (which is to be expected?)

The way this is implemented in the WattTimeDataSource.cs, is that they use a helper class IntervalHelper which is just a friendly wrapper around a clamping filter:
/~https://github.com/Green-Software-Foundation/carbon-aware-sdk/blob/dev/src/CarbonAware.DataSources/CarbonAware.DataSources.WattTime/src/WattTimeDataSource.cs#L72

/~https://github.com/Green-Software-Foundation/carbon-aware-sdk/blob/dev/src/CarbonAware/src/IntervalHelper.cs

Its simple, but maybe we can reuse it here?

[Willmish]

So as discussed in #394 - for now align to the WattTIme DataSource (so return empty array on invalid date range/future date range) using IntervalHelper. Create a new issue to consider throwing exception on requesting future data on the historical endpoints

[YaSuenag]

I pushed new commit to return empty list when ElectricityMapsFree.GetCarbonIntensityAsync() receives request for forecast. AFAICS IntervalHelper expects array of EmissionsData which includes emission datetime, however EMFree seems to be possible to lack it.

carbon-aware-sdk/src/CarbonAware.DataSources/CarbonAware.DataSources.ElectricityMapsFree/src/ElectricityMapsFreeDataSource.cs

Lines 90 to 96 in cfd76fe

	var emissionData = new EmissionsData()
	{
	Location = location.Name ?? "",
	Time = gridEmissionData.Data.Datetime ?? new DateTimeOffset(),
	Rating = gridEmissionData.Data.CarbonIntensity ?? 0.0,
	Duration = new TimeSpan(0, 0, 0)
	};

And also EmissionsHandler would set same value (UtcNow) when startTime and endTime is null.

carbon-aware-sdk/src/GSF.CarbonAware/src/Handlers/EmissionsHandler.cs

Lines 50 to 51 in cfd76fe

	var startTime = parameters.GetStartOrDefault(DateTimeOffset.UtcNow);
	var endTime = parameters.GetEndOrDefault(startTime);

Hence I fixed to check whether periodStartTime is less than current time, and returns empty list if so.

[Willmish]

LGTM, let's discuss how we handle invalid time ranges further in #396