-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Make some analytics call conditional #9061
Conversation
this.sendRequest_(request, trigger); | ||
return request; | ||
}); | ||
return this.checkTriggerEnabled_(trigger, event) |
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.
would it make more sense to check for enabled
before we do the url expansion. That way we prevent some work that might not be used.
@@ -605,6 +612,57 @@ export class AmpAnalytics extends AMP.BaseElement { | |||
} | |||
|
|||
/** | |||
* Checks if request for a trigger is enabled. | |||
* @param {!JSONType} trigger The config to use to determine sampling. |
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.
s/sampling/if the trigger is enabled
|
||
/** | ||
* Checks result of 'enabled' spec evaluation. Returns false if spec is provided and value | ||
* resolves to empty string. |
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 returns false for all falsey values. right? (eg: 0, null and so on)
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.
Method itself always returns Promise, so it's real true/false all the time.
In current implementation false is returned only if the result of a spec expansion is an empty string (assuming that url expansion always returns string, the only falsey value is '').
Should I change this and treat all falsey-looking strings ('0', 'false', 'undefined', 'null, 'NaN'') as false?
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 think we should go ahead and change this, yes. The only more desirable alternative is to change var/url-replacements expansion with a new mode that will take false-y values and instead of outputting them via String(x)
would instead output them as an empty string. However, that'd be a lot of changes for a relatively small feature. So, in the meantime, using falsy strings is a good alternative, I believe.
}; | ||
} | ||
|
||
it('allows a request through', () => { |
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.
allows a request through for undefined `enabled` property
const analytics = getAnalyticsTag(config); | ||
|
||
const urlReplacements = urlReplacementsForDoc(analytics.element); | ||
sandbox.stub(urlReplacements.getVariableSource(), 'get').returns(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.
this is interesting. Does the 0 get converted to a string? how does the tigger get enabled for this case?
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.
The result of expandUrlAsync is always a string because it gets encoded every time, that's the reason 0 (that gets translated to '0') is treated as 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.
Knowing the fact that URL replacements does conversion to strings, we still can fairly easily standardize on what strings lead to false
, including 0
and false
.
}); | ||
}); | ||
}); | ||
|
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.
Can you add a test or two for the case where both tag level and trigger level properties are specified?
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.
Done
ptal |
if (!enabled) { | ||
return; | ||
} | ||
return this.expandExtraUrlParams_(trigger, event) |
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.
Could you move this into a separate method, e.g. expandAndSendRequest_
?
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.
Done
|
||
/** | ||
* Checks result of 'enabled' spec evaluation. Returns false if spec is provided and value | ||
* resolves to empty string. |
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 think we should go ahead and change this, yes. The only more desirable alternative is to change var/url-replacements expansion with a new mode that will take false-y values and instead of outputting them via String(x)
would instead output them as an empty string. However, that'd be a lot of changes for a relatively small feature. So, in the meantime, using falsy strings is a good alternative, I believe.
@zikas Thanks a lot for the PR! This looks very good. I just added a couple of comments, most importantly around the point you've made on falsy strings. |
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.
@dvoytenko No problem, all done. Thank you for the review!
}); | ||
}); | ||
}); | ||
|
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.
Done
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.
just a few small changes. Mostly lgtm from my side.
* @param {string} request The request to process. | ||
* @param {!JSONType} trigger JSON config block that resulted in this event. | ||
* @param {!Object} event Object with details about the event. | ||
* @return {!Promise<string|undefined>} The request that was sent out. |
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 believe type is always !Promise<string>
here. no?
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.
Done
InstrumentationService, | ||
instrumentationServicePromiseForDoc, | ||
AnalyticsEventType, | ||
InstrumentationService, |
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.
nit: were the indentation changes automatically generated?
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.
Yeah, I guess this is my ide. Lint accepts either way. I rolled back unrelated indentation changes.
.then(key => urlReplacementsForDoc(this.element).expandAsync(key)); | ||
return keyPromise | ||
const expansionOptions = this.expansionOptions_({}, trigger); | ||
return this.expandTemplateWithUrlParams_(sampleOn, expansionOptions) | ||
.then(key => this.cryptoService_.uniform(key)) | ||
.then(digest => digest * 100 < spec['threshold']); |
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.
Could you please change here spec['threshold']
to threshold
from above const threshold
?
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.
Done
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.
One small request and LGTM. Thanks!
Thanks again! Looks like everything is green. Merging... |
Added the ability to fire analytics triggers conditional based on
enabled
property that can be specified in individual triggers and at the root level.FIXES #4752