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

User turnstile event #6980

Merged
merged 13 commits into from
Aug 6, 2018
Merged

User turnstile event #6980

merged 13 commits into from
Aug 6, 2018

Conversation

asheemmamoowala
Copy link
Contributor

@asheemmamoowala asheemmamoowala commented Jul 17, 2018

What is in this change?

Just like the mapbox-gl-native mobile SDKs, mapbox-gl-js will send a turnstile event, once per calendar day, when (and only when) the Mapbox access token is set and resources are loaded from a Mapbox API. No turnstile event will be sent for maps that use only third-party styles and tiles. This is the same behavior as the mapbox-gl-native mobile SDKs.

The turnstile event generates a random id that is persisted in the browser's localStorage (when available). No other personal identifiable information is sent as part of this event. The subdomain that processes the event is distinct from the tiles API servers and Mapbox does not connect events between the two. There is no way for us to connect style and tile requests to an individual user.

You can view the part of our privacy policy that covers random identifiers here

Why is this needed?

Today, the way that Mapbox determines distinct users of the gl-js sdk is imprecise. A turnstile event helps get more accurate information about things like:

  • How many users are active on an account monthly. Our licensing incorporates end user limits for commercial applications; this change will allow us to closely track these numbers and make them visible to customers.
  • What versions of gl-js are being used by end-users, and what browsers are being used. This will help us prioritize browser compatibility issues and support for cutting-edge technologies such as WebGL 2.

Additionally, as a company we want to use consistent metrics across all our platforms. This change brings the web platform in sync with our mobile platforms.

Can I opt out of this tracking?

If your use of mapbox-gl-js does not set the access token (using mapboxgl.accessToken) then this code is always disabled. When the access token is set, there is no option to disable the turnstile event.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

cc @jfirebaugh @anandthakker

src/util/ajax.js Outdated
callback(null, xhr.response);
} else {
if (xhr.status === 401 && requestParameters.url.match(/mapbox.com/)) {
callback(new AJAXError(`${xhr.statusText}: you may have provided an invalid Mapbox access token. See https://www.mapbox.com/api-documentation/#access-tokens`, xhr.status, requestParameters.url));
Copy link
Contributor

Choose a reason for hiding this comment

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

This case isn't applicable here.

// mapbox tiles.
if (!config.ACCESS_TOKEN ||
!tileUrls ||
!tileUrls.some((url) => { return /mapbox.c(n)|(om)/i.test(url); })) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use parseUrl here. (This regex will match mapbox-com, and mapbox.com in a query string.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

parseUrl throws, and I'd like to avoid having a try..catch. Even with the authority parsed out of the url, it will need a check for mapbox.com and mapbox.cn.

let pending = false;
//Retrieve cached data
if (localStorageAvailable) {
const data = window.localStorage.getItem(STORAGE_TOKEN);
Copy link
Contributor

Choose a reason for hiding this comment

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

Any use of window.localStorage should be wrapped in try/catch. I know you're preflighting with storageAvailable, but localStorage is wonky and can become unavailable mid-flight if a space quota is filled or the user changes permissions.

@@ -119,3 +125,72 @@ function formatUrl(obj: UrlObject): string {
const params = obj.params.length ? `?${obj.params.join('&')}` : '';
return `${obj.protocol}://${obj.authority}${obj.path}${params}`;
}

const STORAGE_TOKEN = 'mapbox.userTurnstileData';
const localStorageAvailable = storageAvailable('localStorage');
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the failure mode if local storage isn't available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no fallback for saving the last update time locally - it would send multiple requests. Are you thinking that there should be a local variable for lastUpdateTime so that in a given instantiation of Map, this still maintains the once per day frequency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no fallback for saving the last update time locally - it would send multiple requests. Are you thinking that there should be a local variable for lastUpdateTime so that in a given instantiation of Map, this still maintains the once per day frequency?


postData(request, payload, (error) => {
if (!error && localStorageAvailable) {
window.localStorage.setItem(STORAGE_TOKEN, JSON.stringify({
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there's a race condition if postTurnstileEvent is called twice in quick succession. Both calls could result in calling postData before either one receives a response and updates lastSuccess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice catch! There isn't a great way to remedy this other than adding a queue. After a first call, if subsequent calls are ignored until the pending request is complete, a failed request (server down for example) would lose events.

@@ -119,3 +125,72 @@ function formatUrl(obj: UrlObject): string {
const params = obj.params.length ? `?${obj.params.join('&')}` : '';
return `${obj.protocol}://${obj.authority}${obj.path}${params}`;
}

const STORAGE_TOKEN = 'mapbox.userTurnstileData';
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the storage key need to include the access token, for e.g. the embed page?

@@ -64,6 +64,18 @@ export const getArrayBuffer = function({ url }, callback) {
});
};

export const postData = function({ url }, payload, callback) {
if (cache[url]) return cached(cache[url], callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably shouldn't cache POSTs, even in tests.

const lastUpdate = new Date(lastUpdateTime);
const now = new Date();
const daysElapsed = (+now - Number(lastUpdateTime)) / (24 * 60 * 60 * 1000);
pending = pending || daysElapsed >= 1 || daysElapsed < 0 || lastUpdate.getDate() !== now.getDate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any situation where lastUpdate.getDate() !== now.getDate() would be continuously true for some extended period of time (timezone change, leap day, DST cutover, ...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thats possible, but daysElapsed is based on UTC time and would be >=1 or <0 anyways, so it would still send an event.

userId: anonId
}]);

postData(request, payload, (error) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're not returning the result of postData here, so the request can't be cancelled by map.remove() (see #6826). That's probably okay, since the response callback doesn't really communicate with the outside world. But if it's straightforward to implement cancellation, let's do it anyway.

@mb12
Copy link

mb12 commented Jul 18, 2018

@asheemmamoowala Can you please kindly clarify the following?

1.) What is the difference between turnstile event and telemetry data that is collected on the native side (if any)? GL Native collects information pertaining to touches, gestures, geo location when tracking is enabled etc. Does the GL JS also track these and post this with each turnstile event?

  1. What is the estimated bandwidth usage that can be attributed to information posted per turnstile event? Recently the following issue was filed on the native side regarding data usage attributed to telemetry.
    High Telemetry Data Usage mapbox-gl-native#11979

@asheemmamoowala
Copy link
Contributor Author

What is the difference between turnstile event and telemetry data that is collected on the native side (if any)? GL Native collects information pertaining to touches, gestures, geo location when tracking is enabled etc. Does the GL JS also track these and post this with each turnstile event?

@mb12 the turnstile event is a single event when a Map first requests a tile from a Mapbox API server, limited to one per calendar day. GL-JS does not track user interactions (touches and gestures) or geo location. The telemetry data collected by Mobile SDKs includes many more events including intermittent location updates.

What is the estimated bandwidth usage that can be attributed to information posted per turnstile event?

Each turnstile event includes a POST with 6-7 parameters that combine to <200 bytes in size.

@asheemmamoowala asheemmamoowala force-pushed the user-turnstile-event branch 2 times, most recently from f5f5b64 to 35dba09 Compare July 18, 2018 23:10
src/util/ajax.js Outdated
@@ -122,6 +123,24 @@ export const getArrayBuffer = function(requestParameters: RequestParameters, cal
return { cancel: () => xhr.abort() };
};

export const postData = function(requestParameters: RequestParameters, payload: string, callback: Callback<mixed>): Cancelable {
requestParameters.method = 'POST';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: copy/extend requestParameters instead of mutating it

@@ -119,3 +126,108 @@ function formatUrl(obj: UrlObject): string {
const params = obj.params.length ? `?${obj.params.join('&')}` : '';
return `${obj.protocol}://${obj.authority}${obj.path}${params}`;
}

class TurnstileEvent {
STORAGE_TOKEN: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason for this to be an instance member rather than a static member or even just a local constant?

}));
this.pendingRequest = null;
this.pending = false;
this.processRequests();
Copy link
Contributor

Choose a reason for hiding this comment

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

clearing the pending/pendingRequest state and processing the rest of the queue should happen unconditionally (i.e. even if there's an error).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely a bug that the pending.. variables aren't cleared. For processRequests I think it should stop attempting to send requests if there is an error, until the next event is queued - in case of network connection loss, or a server outage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yeah that seems reasonable

t.equal(reqBody.event, 'appUserTurnstile');
t.equal(reqBody.sdkVersion, version);
t.ok(reqBody.userId);

Copy link
Contributor

Choose a reason for hiding this comment

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

Add req.respond(200) so that the TurnstileEvent's pending state has an opportunity to be cleared

t.false(req);

t.end();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests should stub browser.now() and include a test that we do post the turnstile event one day later.

Copy link
Contributor

Choose a reason for hiding this comment

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

They should also test the expected behavior:

  • With and without localStorage available (currently, I think they're testing without it)
  • For a page that remains open > 1 day
  • For a page that is closed and reopened a day later (i.e., the load-from-localStorage path)

Copy link
Contributor Author

@asheemmamoowala asheemmamoowala Jul 20, 2018

Choose a reason for hiding this comment

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

For a page that remains open > 1 day

This is currently treated as a single event until the page requests a new mapbox vector/raster source.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should also test the queuing mechanism.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still need tests for the load-from-localStorage path (which will require stubbing localStorage)

const nextUpdate = this.queue.shift();

// Record turnstile event once per calendar day.
if (this.eventData.lastSuccess) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the expected behavior if localStorage is not available? As of now, it looks like we'd only send a turnstile event once per page in that case, since lastSuccess would never be set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eventData caches the id and success values within a single session. But I see the problem, this value needs to be updated irrespective of localStorageAvailable when a request succeeds.

If localStorage is not available, there may be more than one event sent per calendar day, but the events server de-duplicates those.

const lastUpdate = new Date(this.eventData.lastSuccess);
const nextDate = new Date(nextUpdate);
const daysElapsed = (nextUpdate - this.eventData.lastSuccess) / (24 * 60 * 60 * 1000);
dueForEvent = dueForEvent || daysElapsed >= 1 || daysElapsed < 0 || lastUpdate.getDate() !== nextDate.getDate();
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for the lastUpdate.getDate() !== nextDate.getDate() condition? When would these two dates be equal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

daysElapsed is not perfectly named. It calculates how many 24-hr periods have elapsed since the last update. Notifications should be sent once per calendar day, so the date check allows testing for situations where a user loads the map at 11pm and again at 1am.

Date#getDate() returns just the day of the month, so it would be equal on 6/20 and 7/20. The combined check returns true if the clock has gone backwards (fly east), or forward ( by 24+ hrs or by a calendar date).

Copy link
Contributor

Choose a reason for hiding this comment

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

Date#getDate() returns just the day of the month, so it would be equal on 6/20 and 7/20.

Ahhh 🤦‍♂️

* @param str string to validate.
*/
export function validateUuid(str: ?string): boolean {
return str ? /^[0-9a-f]{8}-[0-9a-f]{4}-[4][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(str) : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

return str && /.../i.test(str);

Copy link
Contributor Author

@asheemmamoowala asheemmamoowala Jul 23, 2018

Choose a reason for hiding this comment

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

Flow complaints that

 - null or undefined [1] is incompatible with boolean [2].
 - string [3] is incompatible with boolean [2].

src/util/util.js Outdated
export function uuid(): string {
function b(a) {
return a ? (a ^ Math.random() * 16 >> a / 4).toString(16) :
//$FlowFixMe
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flow doesn't like the implied array literal conversion in : ([1e7] + -[1e3] + -4e3 + -8e3 + -1e11).

array literal [1] is not a number.

Copy link
Contributor

Choose a reason for hiding this comment

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

^ let's include a comment justifying the $FlowFixMe

@@ -119,3 +126,113 @@ function formatUrl(obj: UrlObject): string {
const params = obj.params.length ? `?${obj.params.join('&')}` : '';
return `${obj.protocol}://${obj.authority}${obj.path}${params}`;
}

class TurnstileEvent {
static STORAGE_TOKEN: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Make these regular variables at file scope.

class TurnstileEvent {
static STORAGE_TOKEN: string;
static localStorageAvailable: boolean;
eventData: Object;
Copy link
Contributor

Choose a reason for hiding this comment

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

eventData: { anonId: ?string, lastSuccess: ?number }

try {
const data = window.localStorage.getItem(TurnstileEvent.STORAGE_TOKEN);
if (data) {
const json = JSON.parse(data);
Copy link
Contributor

Choose a reason for hiding this comment

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

this.eventData = JSON.parse(data);

this.eventData.lastSuccess = nextUpdate;
if (TurnstileEvent.localStorageAvailable) {
try {
window.localStorage.setItem(TurnstileEvent.STORAGE_TOKEN, JSON.stringify({
Copy link
Contributor

Choose a reason for hiding this comment

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

JSON.stringify(this.eventData)

event: 'appUserTurnstile',
created: (new Date(nextUpdate)).toISOString(),
sdkIdentifier: 'mapbox-gl-js',
sdkVersion: `${version}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for string template.

t.false(req);

t.end();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also test the queuing mechanism.

this.eventData.anonId = this.eventData.lastSuccess = null;
}
if (!this.eventData.anonId || !this.eventData.lastSuccess &&
isLocalStorageAvailable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The || should be in parentheses (because && supercedes ||)

src/util/util.js Outdated
export function uuid(): string {
function b(a) {
return a ? (a ^ Math.random() * 16 >> a / 4).toString(16) :
//$FlowFixMe
Copy link
Contributor

Choose a reason for hiding this comment

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

^ let's include a comment justifying the $FlowFixMe

const stub = t.stub(browser, 'now');
stub.onCall(0).returns(today);
stub.onCall(1).returns(laterToday);
stub.onCall(2).returns(tomorrow);
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach to stubbing could be fragile, because it assumes that browser.now is only called once per postTurnstileEvent call. Alternative:

let now = Date.now();

t.stub(browser, 'now').callsFake(() => now);

mapbox.postTurnstileEvent(['a.tiles.mapbox.com']);
const reqToday = window.server.requests[0];
reqToday.respond(200);
let reqBody = JSON.parse(reqToday.requestBody)[0];
t.ok(reqBody.created, new Date(now).toISOString());

now += 1; // later today
mapbox.postTurnstileEvent(['b.tiles.mapbox.com']);
t.ok(window.server.requests.length === 1);

now += (25 * 60 * 60 * 1000); // tomorrow
mapbox.postTurnstileEvent(['c.tiles.mapbox.com']);
const reqTomorrow = window.server.requests[1];
reqTomorrow.respond(200);
reqBody = JSON.parse(reqTomorrow.requestBody)[0];
t.ok(reqBody.created, new Date(now).toISOString());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point of this test is to test the queuing mechanism(as suggested by @jfirebaugh previously(comment). Events are queued correctly triggered consecutively and/or responses are returned with a higher latency. The alternative you suggest is a serial approach that is being tested already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. We should be able to use the more explicit approach to manipulating mock-time though, right?

t.false(req);

t.end();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Still need tests for the load-from-localStorage path (which will require stubbing localStorage)

window.localStorage.setItem(`mapbox.turnstileEventData:${config.ACCESS_TOKEN}`, JSON.stringify({
anonId: uuid(),
lastSuccess: now + ms25Hours // 24-hours later
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to test this by calling postTurnstileEvent twice, with the (stubbed) time being set back between calls, rather than setting localStorage directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you implying that the test should cover the case of the queue receiving events in an out-of-order time sequence? In a single threaded environment, that situation seems highly unlikely. Even if it does happen, it should be counted as a single session IMHO.

Testing with LocalStorage allows verifying that clocks going back in time between sessions are accounted for, where it should be posted as a separate session correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh -- no, I just meant that if possible, we should do this test using calls to postTurnstileEvent (the api being tested) rather than by directly manipulating localStorage (which requires baking more implementation details, like the storage key, into the test)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should do this test using calls to postTurnstileEvent

I believe that within a single session, the clock going backwards should not report a new event - so this cannot be tested with calls to the method directly. To simulate calls across sessions, the best way is to change the access token and clear out the cached eventData

Copy link
Contributor

Choose a reason for hiding this comment

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

this cannot be tested with calls to the method directly

Why not, with something like:

let now = Date.now();
t.stub(browser, 'now').callsFake(() => now);

mapbox.postTurnstileEvent();

// simulate system clock being set backwards
now = now - 1000 * 60 * 60 * 24;

mapbox.postTurnstileEvent();

This is a more direct simulation of the situation we're intending to test, and allows the test to avoid a dependency on implementation details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense to add as a negative test, when clock moves bacward, new event is not set.

t.end();
});

t.test('does not POST appuserTurnstile event second time within same calendar day', (t) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than having some tests rely on state from previous tests, I'd be in favor of making each test self-contained.
This could be done by moving the logic from mapbox.postTurnstileEvent into a method on TurnstileEvent, exporting the TurnstileEvent class, and having each test construct a new instance to test against.

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 understand that it will make the tests self contained, but I think it is better to have a singleton TurnstileEvent for each instance of the map so that there can be a local cache of the eventData. Exposing the entire class without postTurnstileEvent being static would require the callers (Map in this case) to manage an instance of the event which seems unnecessary

Copy link
Contributor

@anandthakker anandthakker Jul 30, 2018

Choose a reason for hiding this comment

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

Exposing the class doesn't preclude having a singleton at the module level:

export class TurnstileEvent {
   ...
   postTurnstileEvent(...) { /* actual logic */ }
}

const _turnstileEvent = new TurnstileEvent();

export function postTurnstileEvent() {
  return _turnstileEvent.postTurnstileEvent(...);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exported the class and updated the test names.

Copy link
Contributor

@anandthakker anandthakker left a comment

Choose a reason for hiding this comment

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

Looks good to me! (One minor issue with test assertions, noted below)

t.equal(reqBody.event, 'appUserTurnstile');
t.equal(reqBody.sdkVersion, version);
t.ok(reqBody.userId);
t.ok(reqBody.created, new Date(tomorrow).toISOString());
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be t.equal. (t.ok only checks that the first argument is truthy). Also, should reqBody be JSON.parse(req.requestBody)[1] rather than [0]?

Copy link
Contributor

Choose a reason for hiding this comment

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

A few other t.ok(x, y)s should be t.equals as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good 👀 @anandthakker !

should reqBody be JSON.parse(req.requestBody)[1] rather than [0]?

Actually req should be window.server.requests[1]. Found and fixed after the t.equal change.

@jingsam
Copy link
Contributor

jingsam commented Aug 1, 2018

There always should have an opt-out option, no matter access-token is set or not. Privacy does matter and you have the responsibility to tell users the purpose of data collection.

@lilykaiser
Copy link

Thanks for your comment @jingsam . We take the responsibility of protecting users' privacy seriously. That's why there is no way for us to connect style and tile requests to an individual end user, and no personally identifiable information is collected in this event other than a random identifier.

you have the responsibility to tell users the purpose of data collection.

As @asheemmamoowala mentioned in the original comment, we will use this data to track how many people are using different versions of gl-js to determine the severity of bugs and performance issues. It will also improve our tracking of how many users a commercial application has. Lastly, it brings our internal calculation of active users on gl-js in line with how we calculate mobile users.

We want to facilitate clear communication between us, developers who use our products, and end users. I hope the above points clarify why Mapbox wants to collect an end user number to our developer community.

We unfortunately can't offer opting out while using Mapbox map tile APIs, because this (anonymous) usage information is important for us to sustainably maintain this service. If opting out is a hard requirement for you, note that Mapbox GL JS may be used with other tile services.

@asheemmamoowala asheemmamoowala merged commit b37dede into master Aug 6, 2018
@asheemmamoowala asheemmamoowala deleted the user-turnstile-event branch August 6, 2018 19:45
@scottgerring
Copy link

Will these localStorage items be cleaned out at some point? We're seeing more and more items stashed in there over time but apparently no removal.

@asheemmamoowala
Copy link
Contributor Author

@scottgerring Removal is not in our current implementation or upcoming plans. Please open an issue with reproducible steps if you think that there are an unreasonable number of items being added to the local storage.

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.

7 participants