-
Notifications
You must be signed in to change notification settings - Fork 240
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
[Beta][Task]15982296: Dynamic config for Sender #1966
Conversation
if (_orgEndpointUrl !== senderConfig.endpointUrl) { | ||
if (_orgEndpointUrl) { | ||
// TODO: add doc to remind users to flush before changing endpoint | ||
_self._buffer.clear(); |
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 don't think we want to clear the buffer, as this could cause the loss of events, the TODO is valid but we just tell them that if they don't existing queued events will also be sent to the new endpoint.
|
||
if (shouldUpdate) { | ||
_self.pause(); | ||
let unsentPayload = _self._buffer.getItems().slice(0); |
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 also need to track the "sent" but not acknowledged items and "move" them as well as if the request that sent them fails they will also need to be re-sent. It might be easier to add a copy
function to the Buffer implementations that way we can test them also.
@@ -590,6 +634,8 @@ export class Sender extends BaseTelemetryPlugin implements IChannelControlsAI { | |||
_self._appId = response.appId; | |||
} | |||
} | |||
//let isOrgEndpoint = responseUrl !== _orgEndpointUrl; | |||
let isOrgEndpoint = true; |
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.
Lets not worry about this case and just document as a "potential" issue that when changing the endpoint if a retry is required that any previous events may (if not dropped) be sent to the new endpoint.
Basically, this is where we say that it you want the events to only be sent to the original endpoint that you should either
- Create a 2nd instance and only use that instance
- Flush and wait for the send to be successful (not sure if we currently has a callback on the flush that returns only after all events are flushed -- we do for 1DS).
@@ -124,6 +129,13 @@ abstract class BaseSendBuffer { | |||
|
|||
return null; | |||
}; | |||
|
|||
_self.deepCopy = (buffer: BaseSendBuffer) => { |
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.
Suggestion: Lets either
- rename this to
deepCopyTo
- reverse the logic so that it copies the passed buffer to this instance (which is the default way I started reading this), as to me deepCopy means either create a deep copy of yourself and return it or create a deepCopy of the passed buffer.
- change this so that it's just
copy
orclone
or something like that where it returns a new instance etc. (just thinking out aloud)
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.
copyFrom
might be another good name for this, but only if the logic is reversed
QUnit.assert.deepEqual(arrBuffer.getItems().length, 7, "arrBuffer length"); | ||
QUnit.assert.deepEqual( sessionBuffer.getItems().length, 6, "copy is deep copy"); | ||
|
||
utlSetSessionStorage(logger, BUFFER_KEY,JSON.stringify([])); |
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.
This should also check that the SessionStorage was updated and contains the expected values
QUnit.assert.deepEqual(sessionBuffer.getItems(), [], "original session storage buffer should be clear"); | ||
QUnit.assert.deepEqual(this._getBuffer(BUFFER_KEY, logger), payload, "session storage should not be changed"); | ||
|
||
utlSetSessionStorage(logger, BUFFER_KEY,JSON.stringify([])); |
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.
Likewise we should check that all of the stuff stored in SessionStorage for the old one has been duplicated in the new one
arrForEach(payload, (val) =>{ | ||
sessionBuffer.enqueue(val); | ||
}); | ||
QUnit.assert.deepEqual(this._getBuffer(BUFFER_KEY, logger), payload, "session storage buffer is set"); |
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.
It should also include items "marked as sent" as that is really where we have a risk of loosing events in this scenario
…pplicationInsights-JS into karlie/dynamicsender
No description provided.