Repository navigation
Conversation
|
Hi @Chin-eng, I tried compiling this locally, and it failed. It looks like the issue isn’t only that the schemas haven’t been released, there are also some compilation issues even when the schemas are published locally. Could you please try publishing the schemas locally and fixing the compilation issues first? |
I hit the same failure. On my machine, gradle was using a cached radar-schemas-commons:0.9.0 jar without the Dexcom classes. After clearing that cache and compiling against a fresh publishToMavenLocal jar, the BUILD was sucessful. I used 0.9.1 locally to dodge the stale 0.9.0 cache. Could you run ./gradlew --stop and delete ~/.gradle/caches/modules-2/files-2.1/org.radarbase/radar-schemas-commons, then publish schemas locally and compile again? This way compilation works on my machine. |
this-Aditya
left a comment
There was a problem hiding this comment.
Thanks @Chin-eng, could you please have a look at the comments inline?
| abstract class DexcomRoute( | ||
| private val userRepository: UserRepository, | ||
| override val maxIntervalPerRequest: Duration = DEFAULT_INTERVAL_PER_REQUEST, |
There was a problem hiding this comment.
I see every subclass passes base url into this constructor, but it only takes userRepository and a Duration, so the module doesn't compile. Do you meant to add some parameter here for base url?
There was a problem hiding this comment.
Sorry, I forgot to stage the DexcomRoute file. I have a lot of unstaged local changes for running the connector locally, and I mixed this file up with those. I added apiBaseUrl.
| ): Sequence<RestRequest> { | ||
| val request = createRequest( | ||
| user, | ||
| "$DEXCOM_API_BASE_URL/${subPath()}", |
There was a problem hiding this comment.
This uses the hardcoded url, so requests can never go to sandbox or configurable url, even when the config says sandbox.
There was a problem hiding this comment.
Yeah I changed it with the apiBaseUrl but forgot to stage the changes.
| user, | ||
| "$DEXCOM_API_BASE_URL/${subPath()}", |
There was a problem hiding this comment.
Same here, we can have the configurable base url so we can request any endpoint, eg. sandbox.
| class DexcomAlertsRoute( | ||
| userRepository: UserRepository, | ||
| apiBaseUrl: String = DEFAULT_API_BASE_URL, | ||
| ) : DexcomRoute(userRepository, apiBaseUrl) { |
There was a problem hiding this comment.
We are passing the base url here, The base class request the second argument as duration but we are passing this as a url string.
This is same for every subclass of dexcom route.
There was a problem hiding this comment.
Yeah, I added the apiBadeUrl as the second argument for the parent class.
| .define( | ||
| SOURCE_URL_CONFIG, | ||
| Type.STRING, | ||
| DexcomRoute.DEFAULT_API_BASE_URL, |
There was a problem hiding this comment.
I don't see this defined anywhere.
There was a problem hiding this comment.
Yeah I added it inside the new dexcomRoute file.
| private val dataRangeApiBaseUrl: String = DEFAULT_API_BASE_URL, | ||
| ) : DexcomRoute(userRepository, dataRangeApiBaseUrl) { |
There was a problem hiding this comment.
The Dexcom route won't compile with the apiBaseUrl param, i think it would be better to keep this param in the base class.
There was a problem hiding this comment.
I put the apiBaseUrl inside the DexcomRoute
| start: Instant, | ||
| end: Instant, | ||
| ): Sequence<RestRequest> { | ||
| val request = createRequest(user, "$dataRangeApiBaseUrl/${subPath()}", "") |
There was a problem hiding this comment.
One the dataRangeApiBaseUrl parameter is shifted to base class, this variable can be renamed.
| user: User, | ||
| ): Sequence<RestRequest> { | ||
| val offset = dexcomOffsetManager.getOffset(route, user) | ||
| val startDate = user.startDate |
There was a problem hiding this comment.
IIRC, right now we can't get backfill data for more than 90 days. Even if we agreed to support 365 days of historical data, it could still fail if the user's start date is more than 365 days, so we should limit this if this is the real case. I am not sure exactly how much we can really backfill now, but please update the logic according to that.
There was a problem hiding this comment.
I'm now using the dataRange window instead of user.startDate. When there is no saved offset, generateRequests calls dataRangeCache.windowFor and starts from that window's start.
| }) | ||
| .collect(Collectors.toList()); | ||
| this.configuredUsers = SequencesKt.toSet(dexcomConfig.getUserRepository().stream()); | ||
| logger.info("Received userTask Configs {}", userTasks); |
There was a problem hiding this comment.
I am not sure, but does userTasks also logs the clent id and secret, we can simply log the task and user counts:
| logger.info("Received userTask Configs {}", userTasks); | |
| logger.info("Configured {} tasks for {} users", userTasks.size(), configuredUsers.size()); |
| val nextOffset = if (dataAge <= Duration.ofDays(7)) { | ||
| maxOffsetTime.plus(OFFSET_BUFFER) | ||
| } else { | ||
| maxOf(maxOffsetTime.plus(OFFSET_BUFFER), request.endDate) |
There was a problem hiding this comment.
Why the next offset is after 12 hours from the newest record, with this logic I think the next 12 hours from the current offset won't be fetched?
There was a problem hiding this comment.
Yeah, I meant to change it later. I was testing something. I was planning to make the poll wait window every 5 minutes. I will change it.
| fun parseDexcomTime(value: String): Instant { | ||
| return try { | ||
| OffsetDateTime.parse(value).toInstant() | ||
| } catch (_: DateTimeParseException) { | ||
| Instant.parse(value) | ||
| } | ||
| } |
There was a problem hiding this comment.
This still fails for timestamps without an offset. Dexcom's docs say records sourced from receivers will not have UTC offsets, and even their own /dataRange example uses a value that our parser can't handle right now. The docs say "systemTime is UTC", so we can parse it as UTC. I think this just needs one more try/catch inside the catch block to parse timestamps without a Z, since they are still in UTC:
LocalDateTime.parse(value).toInstant(ZoneOffset.UTC)| if (window == null) { | ||
| logger.info( | ||
| "Skip {} for {}: no dataRange window", | ||
| route, | ||
| user.versionedId, | ||
| ) | ||
| routeNextRequest[routeKey(route, user)] = Instant.now().plus(BACK_OFF_TIME) | ||
| return emptySequence() | ||
| } | ||
| logger.info( | ||
| "No offsets found for {} {}, using dataRange start {}", | ||
| route, | ||
| user.versionedId, | ||
| window.start, | ||
| ) | ||
| startOffset = window.start | ||
| endDate = minOf(endNow, window.end) |
There was a problem hiding this comment.
Starting at window.start can go before the participant's startDate, so we collect data from before they enrolled. Could we start from the window but not before user.startDate, and fall back to user.startDate when there is no window?
startOffset = window?.start?.coerceAtLeast(user.startDate) ?: user.startDate
endDate = endNow
No description provided.