feat:Add config for omitting the batch timestamp

[gmhewett]

Summary

The mParticle docs describe the batch timestamp:

mParticle adds a top-level batch timestamp — timestamp_unixtime_ms — representing the time the batch was received by mParticle. This field is created by mParticle and should not be set manually when sending data via the Events API.

The same is confirmed in my Reddit question here. However, the docs also says that

NOTE: the SDK sets the batch time to the time the batch was originally created. This relies on the current device date. If you see forwarded data with an old date, please check if the device date is properly set.

For web applications, checking the device date is not feasible because the devices can belong to millions of users, but knowing when mParticle received a batch of events is extremely valuable when considering that a user's device clock is not always accurate.

This PR adds a config setting to instruct the web SDK to omit the batch timestamp from the batch upload request to mParticle. This is an opt-in config, so the existing behavior remains the default.

This PR closes #916 (an issue I opened).

Testing Plan

• Was this tested locally? If not, explain why.
• Tested locally using npm run test
• Manually tested sending a null value for the batch's timestmap_unixtime_ms value -- it seems to be populated by the server

Additional Testing Request

Testing the new bundle script on a demo instance would be great if a maintainer could help with that.

[alexs-mparticle]

@gmhewett Thanks for opening this pull request. I think this is a great feature but I had some feedback. I'm not sure if this applies directly to your use case, but instead of making it a boolean flag, would you consider making this something where a customer could add in their own value for a timestamp? For example, if you needed to "backfill" a batch.

As an example, we provide eventOptions, and you can potentially use this to pass in something like a batchTimeStamp value that could be picked up by the sdkToEventsApiConverter.convert method. This could allow you to pass a custom timestamp or even null to omit.

I made some minor comments to about omitting the dist folder from your PR but I may need some more time to dig through your code to test and give you more in-depth feedback. Thanks again!

[gmhewett]

Hey @alexs-mparticle thanks for such a quick review! Removed those files that are generated during the build 👍

I'm not sure if this applies directly to your use case, but instead of making it a boolean flag, would you consider making this something where a customer could add in their own value for a timestamp? For example, if you needed to "backfill" a batch.

I think this would definitely fit my use case, as I really just need a way to make sure a batch timestamp isn't included in the request.

But, it seems like eventOptions is just for event-level config, right? I don't see a way right now to hook into this event-level config to change the batch-level timestamp (and since batches are fired automatically, not from a call from the application).

But in general I like the idea of specifying a timestamp if desired, but I'm not sure how to make it cleanly backwards compatible (assuming only a minor version bump). If a customer upgrades the package without supplying a batchTimestamp value, then the sdk would (perhaps unexpectedly) stop sending the timestamps.

What do you think? Also thanks again for taking a look!

[gmhewett]

Hey @alexs-mparticle! Checking in to see if you had any more thoughts about this change and my comment above. My team has some work streams that may change depending on if/when this feature would be available, so any hint about the timeline would be super helpful. Still happy to help with changes 😄 Thanks!

[alexs-mparticle]

Hey @gmhewett. Sorry for the delay in getting back to you.

You are correct in that eventOptions are an event-level flag, but within the sdkToEventsApiConverter, we do have several cases where an event-level property can be applied to the batch.

For example, when consent is applied to an event, we then apply the consent state from the most recent event to the batch that contains it.

My thought is that you can make a check the of the lastEvent for the eventOption when setting timestamp_unixtime_ms, and if it exists, use that value instead of new Date().

However, to support this, you may need to add a new property to the event itself as events are converted to a ServerModel before being uploaded.

Another option might be to add the overridden timestamp to the store along side the flag you've created, then you can apply it to the batch within sdkToEventsApiConverter.

Let me know if this makes sense. I may need some time to provide more concrete code samples if you require them.

[gmhewett]

@alexs-mparticle I see what you mean, thank you for the detailed explanation!

However, I wasn't sure about using eventOptions for this, as this object isn't pass along in createEventObject in serverModel.ts. In sendEventToServer, the options are only used for determining if the event should be queued, and I didn't think it made sense to change the function signature of queueEventForBatchUpload.

Let me know what you think. Very happy to make changes, let me know if there is something more fundamental we should change about how eventOptions are passed around (or if I'm misunderstanding something).

[gmhewett]

Oh, actually, my proposed solution will not work in the logEvent case. Let me re-think this, I see your point about using eventOptions — I guess we should also check for the value there.

[gmhewett]

Okay, sorry for the noise there, but is this what you were thinking @alexs-mparticle?

I do slightly prefer using an SDK-wide config for this. If the client code has multiple calls to logEvent or logEventBase, then the caller needs to remember to include this everywhere, and forgetting it would be easy when adding new calls to logEvent or logEventBase.

Another option might be to add the overridden timestamp to the store along side the flag you've created, then you can apply it to the batch within sdkToEventsApiConverter.

Is there an easy way to change this value in the store after init? I'm thinking it would be nice to keep the omitBatchTimestamp flag around to control this at a global level.

Thanks again for your review, lmk what you think.

[alexs-mparticle]

No worries about the noise. Sometimes it's good to get the ideas out of your head.

The _Store is kind of our "catch all" state for the SDK's runtime, and we use it when we need to share or update state between modules, such as this example in our Session Manager.

I personally would prefer your timestamp to exist in a "Batch Manager", which we don't have at the moment, so _Store is probably the best place for the time being.

I can potentially see you storing both the omitBatchTimestamp flag as well as a overrideBatchTimestamp value in the _Store and then within sdkToEventsApiConvert you can check for both, if there's a valid use case.

Keep any and all questions coming!

[gmhewett]

I can potentially see you storing both the omitBatchTimestamp flag as well as a overrideBatchTimestamp value in the _Store and then within sdkToEventsApiConvert you can check for both, if there's a valid use case.

Cool, I've implemented this strategy:

The SDKConfig (and InitSDKConfig) has a new parameter omitBatchTimestamp, and the application can set this as a default config setting when initializing mParticle.

The store also has batchTimestampUnixtimeMsOverride parameter—this can be set by the application at any time to control the batch timestamp that is used.

batchTimestampUnixtimeMsOverride takes precedence over omitBatchTimestamp, as a setting configured later should take precendence over an initialize config IMO. Example: an application chooses to not send timestamps with omitBatchTimestamp, but if there is some strong signal from the application that some offline events are ready, the application could set batchTimestampUnixtimeMsOverride in this case (w/o re-initializing, which would be a pain).

Thanks @alexs-mparticle for your comments, please let me know what you think!

[alexs-mparticle]

This looks great! I'm going to need some time to pull these changes locally and make sure they pass our tests but right now my only comments are more based around house styling and our own conventions, so I hope my feedback isn't too hard to address.

[alexs-mparticle]

We're trying to get into the habit of being more explicit and intentional with typing of config settings and overrides, so I would ask if you can add SDKConfig and Store as the type here.

[gmhewett]

Makes sense. I actually did that at first, but I didn't want to give the impression that any field could be overridden via this config, as I only implemented the overrides for these two fields.

I changed it to use Partial<Pick<SDKConfig, 'omitBatchTimestamp'>> and Partial<Pick<IStore, 'batchTimestampUnixtimeMsOverride'>>, which uses the derived type but ensures that only the implemented fields can be overridden.

Lmk what you think.

[alexs-mparticle]

As much as I love ternary operations, but sometimes they can be hard to read, especially when they're nested and have long variable names.

I'd be fine with you keeping it if you destructure the members or just move it above the upload declaration and make it a traditional if/else statement.

[gmhewett]

Cool, sounds good.

Also, I want to confirm one thing: I'm just checking for a truthy value on batchTimestampUnixtimeMsOverride, so callers want be able to send batchTimestampUnixtimeMsOverride: 0. This is fine for me (as this would likely be a mistake), but just wanted to call it out.

[gmhewett]

This looks great! I'm going to need some time to pull these changes locally and make sure they pass our tests but right now my only comments are more based around house styling and our own conventions, so I hope my feedback isn't too hard to address.

Sounds good, thanks for taking a look. I've fixed all your comments. Any tips on how to test this in a demo app or something?

[rmi22186]

Hi @gmhewett - Alex is on PTO so I can chime in here. This depends on how you load mParticle into your test app.

• CDN - Are you familiar with overrides? If you are loading mParticle via snippet/CDN, you can override the mparticle.js file with a locally built dist/mparticle.js file following the link's instructions.
• npm - you can run npm build:npm to build the commonjs file or npm build:esm for the esm file, then follow the npm link docs in order to have your locally built app import the local mparticle module. That should get you started. Let me know if you have any issues.

[gmhewett]

Thanks @rmi22186 and @alexs-mparticle!

I've tested both of these flags out myself and everything looks good from my side. Lmk if you'd like anything else done for testing before we merge this. Thanks!

[gmhewett]

Hi @alexs-mparticle @rmi22186 — wanted to check and see what your thoughts on on this change. Any idea of if/when it might be able to get merged and released?

Let me know if you need anything else from me. Gettin accurate server-side timestamps is important for a project I'm working on, so it would be great to have some estimation on the timeline.

Appreciate your time, thanks!

[alexs-mparticle]

@gmhewett Sorry for any delays. I tried pulling your code and found that a lot of our tests are failing. You're likely going to have to rebase your branch against our latest development branch to get our latest test updates. Can you try doing that and let me know so I can test again?

Overall everything looks good so if we can verify the tests pass, we should be able to include this in an upcoming release.

[gmhewett]

@gmhewett Sorry for any delays. I tried pulling your code and found that a lot of our tests are failing. You're likely going to have to rebase your branch against our latest development branch to get our latest test updates. Can you try doing that and let me know so I can test again?

@alexs-mparticle No worries, thanks for taking a look! I've re-based against development and (force-) pushed the changes to this branch. I tested everything locally and things seem to work fine after the rebase.

Overall everything looks good so if we can verify the tests pass, we should be able to include this in an upcoming release.

Cool, let me know if you run into any more issues. Thanks a ton!

[alexs-mparticle]

@gmhewett I have approved your PR and verified that it passes all tests. I'll reply back with an estimated release date when I check in with the team.

[gmhewett]

@gmhewett I have approved your PR and verified that it passes all tests. I'll reply back with an estimated release date when I check in with the team.

Thanks @alexs-mparticle! Really appreciate it 😄

[alexs-mparticle]

@gmhewett Sorry to move the goal post on you so late in the game but I noticed that batchTimestampUnixtimeMsOverride wasn't actually being set anywhere at the start of the logEvent workflow. Let me know if this seems like an easy fix to you, other wise I can jump in to assist.

[alexs-mparticle]

I just noticed that you're puling this from _Store as we discussed, but I don't see any code path where batchTimestampUnixtimeMsOverride is being added to the actual _Store. If I remember correctly, we discussed that the override would be added as an eventOption.

I think within APIClient.sendEventToServer, you can pass in batchTimestampUnixtimeMsOverride through _options and use that to set it in the _Store, where it can then be picked up here.

[rmi22186]

Reminder that it should be added to each of logPageView, logEvent, and logProductAction.

Per your and my comments, I am striking out the above.

[gmhewett]

@alexs-mparticle @rmi22186 no worries! I've just pushed up the change.

@rmi22186, you mentioned:

Reminder that it should be added to each of logPageView, logEvent, and logProductAction.

But I think updating sendEventToServer is enough, as all of these log* functions use sendEventToServer, right?

[rmi22186]

@gmhewett You're right! For some reason I thought we were adding something to the API, but we are just leveraging an existing API (the options, which is freeform) to add this. So you're fine there. I'll let Alex leave you with the remaining feedback he has.

[alexs-mparticle]

Thanks for the update. I only have one minor change to your conditional, but everything else looks great to me.

Regarding the release timeline, normally we would just ship this feature during our next release window but given that we are entering our blackout period this week, I will need to prioritize your feature with our product team.

Given that your feature isn't making huge changes, there shouldn't be any blockers, but we want to be extra careful during this blackout period. I apologize if it feels like we keep moving the goal posts, but we really want to make sure we limit any risks to other customers when shipping code.

Can you please email [email protected] and make a request for us to release this feature for you? Please be sure to include your customer information as well as the name of your CSM (if you are working with one). Also tell them know you were working with @rmi22186 and me and reference this PR so they have context.

Once I have that, I can get you a more accurate release window. Thanks again for your contribution and I really enjoyed reviewing your code. You made some great design decisions and we were very happy to work with you!

[alexs-mparticle]

We have a utility function called isNumber that checks if the type is boolean which is more suited for this kind of check.

Just don't forget to import it from /src/utils 😄

Suggested change

	if (options.batchTimestampUnixtimeMsOverride !== undefined) {
	if (isNumber(options.batchTimestampUnixtimeMsOverride) {

[gmhewett]

Nice, I think you suggestion is a good one—but I think it should be applied in src/sdkToEventsApiConverter.ts instead.

In src/apiClient.ts, I think it's important that we allow the caller to "unset" a batch timestamp override. If we check isNumber(options.batchTimestampUnixtimeMsOverride)) here, then the caller won't be able to remove the override and let the current timestamp (or null in the case of omitBatchTimestamp) to be used.

So, I changed the flow slightly to

1. In src/apiClient.ts, allow a value in eventOptions to be used to set the batchTimestampUnixtimeMsOverride in the store.
2. In src/sdkToEventsApiConverter.ts, only if isNumber(batchTimestampUnixtimeMsOverride) will we apply the override.

So setting batchTimestampUnixtimeMsOverride: undefined in eventOptions will "clear" the override.

What do you think?

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[gmhewett]

Thanks for the update. I only have one minor change to your conditional, but everything else looks great to me.

Thank you! I made one slight modification to your suggestion (and left some details in the corresponding comment)—let me know if you are good with this. If not, happy to make an adjustment.

Regarding the release timeline—that all makes sense, and thank you for the detailed explanation! Once you approve the PR, I'll have someone from our team kick off that conversation with [email protected]

Thanks again for your contribution and I really enjoyed reviewing your code. You made some great design decisions and we were very happy to work with you!

Appreciate it! It's a nice library, and thanks for being open to PRs and providing such detailed comments!

[gmhewett]

@alexs-mparticle @rmi22186 FYI, we ended up using a workaround that support gave us:

const mParticleConfig = {
  onCreateBatch: (batch) => {
    batch.timestamp_unixtime_ms = undefined;
    return batch;
  },
};

And so far it seems to be working 👍 not sure if you still want to merge this PR?

[alexs-mparticle]

@gmhewett Glad to know the workaround is working for you and I appreciate all the work you put in with the PR.

I think we're going to forgo merging this PR since the workaround solved your initial issue, but we'll keep the branch around so we can evaluate it for a future revision to our batches.

I've got some refactors planned for the near future and I think I can possibly incorporate some of your code changes into that update.

Once again, thank you for all of your work and putting up with all of the back and forth. You came up with some great ideas and I enjoyed working with you.