-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: sdkConfig and lastModified - FDN-281 #154
base: main
Are you sure you want to change the base?
Conversation
@@ -143,6 +148,12 @@ private ProjectConfig getConfigResponse(Call<ProjectConfig> call) throws DevCycl | |||
} | |||
} | |||
this.configETag = currentETag; | |||
this.configLastModified = lastModified; | |||
try { | |||
this.eventQueueManager.queueSDKConfigEvent(call.request(), response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I remember the reason I had to avoid tying these things together being circular dependency issues. It's probably not a big issue here, but might it be better to try not to tie these two main managers together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no circular dep issues here; they're distinct and don't relate - tying them together just is a better/cleaner practice than creating a confusing backchannel.
it's the same structure as c# and Go
if (res.isSuccessful()) { | ||
event.setValue(BigDecimal.valueOf(res.networkResponse().receivedResponseAtMillis() - res.networkResponse().sentRequestAtMillis())); | ||
} else { | ||
event.setValue(BigDecimal.valueOf(-1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should still be able to get a response time if the request fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't track the event if it's not successful - its not really needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not all cases return a valid response (ie timeout)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do want to track the event if its not successful. that's why there is a errMsg
in the metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can switch it up - but I think it'd be more confusing the way it'd have to be structured
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the main reason for this event is to really track the different errors the SDKs receive, so it's kinda a requirement here.
Auto formatter went through - not me manually.