USACE / cwms-data-api

Corps Water Management System RESTful Data Service
MIT License
11 stars 14 forks source link

Values returning different for Internal vs External CDA instances for a specific Project / Timeseries #556

Open krowvin opened 6 months ago

krowvin commented 6 months ago

Steps to reproduce the problem

  1. GET request on PINE.Elev.Inst.1Hour.0.Ccp-Rev version 1 or 2 on CDA internal instance

  2. GET request on same timeseries on water.usace.army.mil or CWBI PROD

    'https://cwms-data.usace.army.mil/cwms-data/timeseries?name=PINE.Elev.Inst.1Hour.0.Ccp-Rev&office=SWT&timezone=US%2FCentral' \
    -H 'accept: application/json;version=2'
  3. Compare the first value for the same timezone using the default 24 hours of data response against the public endpoints

What you expected to happen: Expect values to match for same request

What actually happened (or didn't happen) ~External instance of CDA appears to return data off by 6 hours (TimeZone difference)~

Internal CDA version is: 3.1.2

Things I tried for diagnosing Spot checking other timeseries for this project look correct.

Checked the same process using version 1 of the API. Same issue.

Confirmed the PINE location metadata is set to (US/Central)

MikeNeilson commented 6 months ago

Please paste the exact query/http request in (if using swagger the curl command is the best thing to paste in.)

That said, while the API can take times in different timezone it does, and always will return things in UTC (or at least it should) so given that two different instances have returned different times that appears to be an actual bug you've found.

This may have been fix in some recent PRs as we found some other TZ handling issues. Please put in the exact request to make it easier to diagnose.

krowvin commented 6 months ago

Done. Post updated and external curl added.

DanielTOsborne commented 6 months ago

I think it might be something else than timezone, and I'm not sure if it's even CDA related. Here's a query I tested to the public instance: https://water.usace.army.mil/cwms-data/timeseries?name=PINE.Elev.Inst.1Hour.0.Ccp-Rev&office=swt&timezone=America/Chicago Change the query to point to the internal SWT instance (url not posted for obvious reasons), and you get completely different, but still similar data.

image

The left side is the public instance data returned, right side is the internal instance. It almost looks like it's off by one hour, except the data doesn't entirely match, even accounting for floating point rounding errors.

rma-rripken commented 6 months ago

I'm at a loss.
I just ran the test TimeseriesControllerTestIT.test_lrl_1day in the debugger and I don't see any timezone issues with how values are being converted either going into the database using CDA JSONv2 POST or coming out of it using JSONv2. The test inserts two daily values. Then queries using a start==end time range of the first value and asserts that it gets back the correct value. While the test was running I manually queried the AV_TSV_DQU and verified the DATE_TIME column. If there was a one-way shift on insert or retrieve the integration test would have failed. If there was a shift on insert that was undone on retrieve the manual inspection would have caught that.

I did not test with the v1 end-points.

rma-rripken commented 6 months ago

The documentation on the "timezone" query parameter isn't great. For JSONv1 that parameter gets passed to the pl/sql - we need to add integration tests for the JSONv1 behavior.
For JSONv2 the timezone parameter is only used if the user didn't include a timezone in their begin or end strings. Its weird. In my opinion we should drop the timezone param from all the v2 end-points and require the user to provide begin and end strings that include timezone info. If their date input can't be parsed its an exception. If they provided an extra timezone parameter to V2 that should also be an exception. I wonder if we should look for any unhandled/extra parameters and throw an exception if extra's are found... I bring this all up b/c the original url is a weird one to use for JSONv2. It doesn't have a begin or end parameter, so it will use the defaults and the timezone field will be ignored. We probably need to better document the V1 vs V2 quirks