Skip to content

Commit

Permalink
[superset-client][logger] replace ajax with SupersetClient (apache#6133)
Browse files Browse the repository at this point in the history
* [superset-client][logger] replace ajax with SupersetClient

* [superset-client][logger] don't parse log response
  • Loading branch information
williaster authored Oct 23, 2018
1 parent fc3b68e commit 8573fde
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 54 deletions.
91 changes: 51 additions & 40 deletions superset/assets/spec/javascripts/logger_spec.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import $ from 'jquery';
import sinon from 'sinon';
import fetchMock from 'fetch-mock';

import { Logger, ActionLog } from '../../src/logger';

Expand Down Expand Up @@ -43,10 +42,15 @@ describe('ActionLog', () => {
log.addEvent(eventName, eventBody);
expect(log.events[eventName]).toHaveLength(1);
expect(log.events[eventName][0]).toMatchObject(eventBody);
Logger.end(log);
});
});

describe('Logger', () => {
const logEndpoint = 'glob:*/superset/log/*';
fetchMock.post(logEndpoint, 'success');
afterEach(fetchMock.resetHistory);

it('should add events when .append(eventName, eventBody) is called', () => {
const eventName = 'testEvent';
const eventBody = { test: 'event' };
Expand All @@ -59,30 +63,24 @@ describe('Logger', () => {
});

describe('.send()', () => {
beforeEach(() => {
sinon.spy($, 'ajax');
});
afterEach(() => {
$.ajax.restore();
});

const eventNames = ['test'];

function setup(overrides = {}) {
const log = new ActionLog({ eventNames, ...overrides });
return log;
}

it('should POST an event to /superset/log/ when called', () => {
it('should POST an event to /superset/log/ when called', (done) => {
const log = setup();
Logger.start(log);
Logger.append(eventNames[0], { test: 'event' });
expect(log.events[eventNames[0]]).toHaveLength(1);
Logger.end(log);
expect($.ajax.calledOnce).toBe(true);
const args = $.ajax.getCall(0).args[0];
expect(args.url).toBe('/superset/log/');
expect(args.method).toBe('POST');

setTimeout(() => {
expect(fetchMock.calls(logEndpoint)).toHaveLength(1);
done();
}, 0);
});

it("should flush the log's events", () => {
Expand All @@ -97,7 +95,7 @@ describe('Logger', () => {

it(
'should include ts, start_offset, event_name, impression_id, source, and source_id in every event',
() => {
(done) => {
const config = {
eventNames: ['event1', 'event2'],
impressionId: 'impress_me',
Expand All @@ -111,39 +109,52 @@ describe('Logger', () => {
Logger.append('event2', { foo: 'bar' });
Logger.end(log);

const args = $.ajax.getCall(0).args[0];
const events = JSON.parse(args.data.events);

expect(events).toHaveLength(2);
expect(events[0]).toMatchObject({
key: 'value',
event_name: 'event1',
impression_id: config.impressionId,
source: config.source,
source_id: config.sourceId,
});
expect(events[1]).toMatchObject({
foo: 'bar',
event_name: 'event2',
impression_id: config.impressionId,
source: config.source,
source_id: config.sourceId,
});
expect(typeof events[0].ts).toBe('number');
expect(typeof events[1].ts).toBe('number');
expect(typeof events[0].start_offset).toBe('number');
expect(typeof events[1].start_offset).toBe('number');
setTimeout(() => {
const calls = fetchMock.calls(logEndpoint);
expect(calls).toHaveLength(1);
const options = calls[0][1];
const events = JSON.parse(options.body.get('events'));

expect(events).toHaveLength(2);

expect(events[0]).toMatchObject({
key: 'value',
event_name: 'event1',
impression_id: config.impressionId,
source: config.source,
source_id: config.sourceId,
});

expect(events[1]).toMatchObject({
foo: 'bar',
event_name: 'event2',
impression_id: config.impressionId,
source: config.source,
source_id: config.sourceId,
});

expect(typeof events[0].ts).toBe('number');
expect(typeof events[1].ts).toBe('number');
expect(typeof events[0].start_offset).toBe('number');
expect(typeof events[1].start_offset).toBe('number');

done();
}, 0);
},
);

it(
'should send() a log immediately if .append() is called with sendNow=true',
() => {
(done) => {
const log = setup();
Logger.start(log);
Logger.append(eventNames[0], { test: 'event' }, true);
expect($.ajax.calledOnce).toBe(true);
Logger.end(log);

setTimeout(() => {
expect(fetchMock.calls(logEndpoint)).toHaveLength(1);
Logger.end(log); // flush logs
done();
}, 0);
},
);
});
Expand Down
23 changes: 9 additions & 14 deletions superset/assets/src/logger.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
/* eslint no-console: 0 */

import $ from 'jquery';
import { SupersetClient } from '@superset-ui/core';

// This creates an association between an eventName and the ActionLog instance so that
// Logger.append calls do not have to know about the appropriate ActionLog instance
Expand Down Expand Up @@ -41,13 +40,13 @@ export const Logger = {

send(log) {
const { impressionId, source, sourceId, events } = log;
let url = '/superset/log/';
let endpoint = '/superset/log/?explode=events';

// backend logs treat these request params as first-class citizens
if (source === 'dashboard') {
url += `?dashboard_id=${sourceId}`;
endpoint += `&dashboard_id=${sourceId}`;
} else if (source === 'slice') {
url += `?slice_id=${sourceId}`;
endpoint += `&slice_id=${sourceId}`;
}

const eventData = [];
Expand All @@ -63,14 +62,10 @@ export const Logger = {
});
}

$.ajax({
url,
method: 'POST',
dataType: 'json',
data: {
explode: 'events',
events: JSON.stringify(eventData),
},
SupersetClient.post({
endpoint,
postPayload: { events: eventData },
parseMethod: null,
});

// flush events for this logger
Expand All @@ -88,7 +83,7 @@ export class ActionLog {
this.impressionId = impressionId;
this.source = source;
this.sourceId = sourceId;
this.eventNames = eventNames;
this.eventNames = eventNames || [];
this.sendNow = sendNow || false;
this.events = {};

Expand Down

0 comments on commit 8573fde

Please sign in to comment.