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

Adding Logger module to track events #99

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Adding Logger module to track events #99

wants to merge 21 commits into from

Conversation

cmchin
Copy link

@cmchin cmchin commented Aug 4, 2015

Questions:

  • what if theres a start signal but no end signal? How long should we wait before we assume that its the end of the session and something went wrong?
  • How much can I assume that evenInfo can has all the keys that I want it to? (For example, how do I handle events that don't have a uuid or session id?)
  • Should I not send information to new relic in logger if we're still not sure if we're going to buy it?
  • How do I minimize the amount of code I copy and paste from backbone/torso?

* @ param {JSON object} contains tracking information (including state: 'start' or 'end', uuid, sessionID)
* @ method track
*/
track: function(eventInfo){
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation issue

@kentmw
Copy link
Contributor

kentmw commented Aug 4, 2015

To answer your questions:

  1. I haven't thought too much on this, but having a start and no end is useful information to capture.
  2. I think you should be defaulting as much as possible. A stop call though needs a UUID
  3. newrelic sending needs to be an option, not a requirement regardless if Vecna purchases it.
  4. We need to prevent copying from backbone. I think we can live with wrapping the original calls with tracking calls. We don't need the granularity of putting tracking calls inside the original methods. It should look something like:
fetch: function() {
  var eventId = Logger.track();
  Backbone.Model.prototype.fetch.apply(this, arguments);
  Logger.track(eventId);
}

This commit brings up the question of how much can we make added functionality (like sending to new relic) pluggable on top of torso.

/**
* Overridden to send before and after signals to Logger
*/
fetch: function(options){
Copy link
Contributor

Choose a reason for hiding this comment

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

look into $(document).ajaxStart and $(document).ajaxComplete

if (!callback) callback = this[name];
var callbackCopy = function(){
var uuid = (new Date()).getTime().toString(16)+Math.floor(1E7*Math.random()).toString(16);
this.trigger('routeTiming',
Copy link
Contributor

Choose a reason for hiding this comment

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

See all the comments about these type of event names and payloads.

Note, the convention of triggered events are "-" separated (not camelCased) amd that they describe what has or about to happen.

@kentmw
Copy link
Contributor

kentmw commented Aug 10, 2015

Great work @cmchin! I think you did a good job reducing the rewriting backbone. There's a few changes are left in order for across-the-board value from your commit.

Can you create test cases for this commit?

removed eventTracker
changed payload information
added event listeners and triggers in both router/view/model instance and Torso.Events
removed eventTracker
changed payload information
added event listeners and triggers in both router/view/model instance and Torso.Events
@cmchin
Copy link
Author

cmchin commented Aug 12, 2015

@kentmw @mandragorn @catbieber

Made all the changes mentioned above:

  • changed payload of events
  • got rid of EventTracker
  • Events are triggered on the object itself (this). Each view/router/module listens for timing events on itself and triggers an event on Torso.Events


constructor: function(){
Backbone.Router.apply(this, arguments);
this.__applyTimingEventListeners(['pre-route', 'post-route']);
Copy link
Author

Choose a reason for hiding this comment

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

@kentmw is there a better way to add all event listeners without enumerating each one as strings in the constructor?

Copy link
Contributor

Choose a reason for hiding this comment

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

@cmchin, I can't think of one short of hijacking the events logic and checking for "pre-" and "post-" callbacks and triggering the Events event there. Although, I could see you passing in pairs of strings to which it can handle Events-specific logic across events (passing information from the start to the stop if needed).

I like being declarative about which events we are binding to timing events, at least for now.

@kentmw
Copy link
Contributor

kentmw commented Aug 13, 2015

@cmchin super close and really useful 👍

@kentmw
Copy link
Contributor

kentmw commented Sep 9, 2015

@mandragorn , @cmchin I'm excited about this pull request. Any status on the last touches?

@kentmw
Copy link
Contributor

kentmw commented Oct 15, 2015

@cmchin do you want to finish this patch? If I don't hear from you in a week, I'll just finish it for you.

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.

4 participants