Skip to content
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

Stamp Specific endpoint changes #1474

Merged
merged 9 commits into from
Mar 2, 2021
Merged

Stamp Specific endpoint changes #1474

merged 9 commits into from
Mar 2, 2021

Conversation

kryalama
Copy link
Contributor

@kryalama kryalama commented Feb 12, 2021

Overview

  • Ingestion endpoint redirects incoming requests where URL doesn’t match the IngestionEndpoint of a given ikey. For requests with a single ikey, Breeze sends back 308 (permanent redirect) response code with a Location header specifying the URL where the client should send the data instead. For requests with multiple ikeys, Breeze sends back 206 (partial content) response code with the redirection URLs embedded in the response body.(Partial redirects is not a valid scenario for Web JS SDK)
  • SDKs will post requests to the new /v2.1/track query path to indicate support for redirects from Breeze. (work in progress from Ingestion side)
  • SDKs will follow up to 10 redirects (to prevent circular redirects)

Internal Change spec

//TODO

  1. Once we have the new api ' /v2.1/track' available , make necessary updates to SDK.
  2. Add unit tests if possible
  3. Once Breeze have the ability to accept query parameter 'redirect=false', update the beacon API code and the snippet code to call the new url with redirect=false parameter.

@kryalama kryalama requested a review from MSNev February 12, 2021 01:41
@MSNev
Copy link
Collaborator

MSNev commented Feb 12, 2021

Note: it looks like the test pass failed for the same issue I talked about in standup today.

My PR #1469 has the temporary fix for this -- it's a bit big though, but if you have a chance to review would be good.


// check if the xhr's responseURL is same as endpoint url
// TODO after 10 redirects force send telemetry with'redirect=false' as query parameter.
if(xhr.responseURL !== _self._senderConfig.endpointUrl()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should check that

  • the responseURL is not empty and look valid (some XHR polyfill's might not set this etc)

Additional considerations.

  • Do we want to retry the original URL if the redirected one also continues to fail
  • Do we only want to update on a successful send (200-299 response status code)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think updating the url only after we get a successful xhr send is a better approach.

@MSNev
Copy link
Collaborator

MSNev commented Feb 17, 2021

Note: it looks like the test pass failed for the same issue I talked about in standup today.

Fixes for the test failures (dts generation) are now checked in, to rebasing should resolve the test issues

@kryalama kryalama requested a review from MSNev February 25, 2021 23:38
@@ -386,6 +400,11 @@ export class Sender extends BaseTelemetryPlugin implements IChannelControlsAI {
_InternalMessageId.TransmissionFailed, `. Offline - Response Code: ${xhr.status}. Offline status: ${Offline.isOffline()}. Will retry to send ${payload.length} items.`);
}
} else {

// check if the xhr's responseURL is same as endpoint url
// TODO after 10 redirects force send telemetry with 'redirect=false' as query parameter.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the timeframe for this todo?

@MSNev MSNev added this to the 2.6.0 milestone Feb 26, 2021
@kryalama kryalama merged commit cbe1546 into master Mar 2, 2021
@MSNev MSNev deleted the kryalama/sse branch August 4, 2021 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants