search

segment-integrations / analytics.js-integration-facebook-pixel

The Facebook Pixel analytics.js integration. https://segment.com/docs/integrations/facebook-pixel
MIT License
8 stars 16 forks source link

Add agent support #12

Closed vincentchio closed 8 years ago

vincentchio commented 8 years ago

@hankim813, this change adds agent option support.

f2prateek commented 8 years ago

LGTM, but why would we want to let uses change the agent? This is should always be segment, no?

vincentchio commented 8 years ago

Facebook also wants to identify shopify in the event so they can do customization. This option would allow others to take advantage of custom agent.

hankim813 commented 8 years ago

LGTM also, we need to update metadata so that the option will be displayed in the UI settings for FB. @jgershen wanna take care of this? :D

sperand-io commented 8 years ago

@vincentchio Shopify doesn't use the hosted version of ajs, correct? Our understanding w FB was that they wanted all segment-hosted fb pixel integrations to self-identify as coming from Segment. It makes sense that other platforms may need to self-identify, but we don't want our customers overriding the agent with random strings in their settings panes :)

With that in mind, would it make sense for the Shopify fork to just hardcode the agent and leave the field as is? Alternatively, if you all rely on the Segment repo, we can merge this in and just not add the setting to our integration metadata since Shopify can initialize with whatever settings you wish.

In any case, glad to see you all were able to use analytics.js to add FB Pixel support in addition to GA easily! Another discussion I'd love to have is whether it might make sense to optionally avail Shopify customers of the hosted version of ajs through dedicated Segment accounts so that they can take advantage of all our integrations, but that's probably best covered with our respective BD teams... :)

Let me know your thoughts!

vincentchio commented 8 years ago

@sperand-io Shopify does not currently use the hosted version of Analytics.js. In terms of integrating with hosted version, I would leave it to our respective BD teams.

I could imagine that other consumers of the Analytics.js library without the hosted version would also want to set the agent so that they can be properly identified by Facebook. It makes sense to me to have this ability exposed as an option since Analytics.js is a open source library and the seg agent should not be hard-coded there. I think of the hosted version as an implementation of Analytics.js offered as a service by SegmentIO and if it does not make sense for consumer to change agent, it can simply disable the functionality. We should allow the flexibility in the api but implement constraints in the custom services.

Thanks!

f2prateek commented 8 years ago

@vincentchio The issue with with us exposing it for all our hosted users is that this would inhibit Facebook from attributing events to segment for users that choose to override it. As Chris mentioned, consumers of non hosted segment (such as shopify and others) can use a fork to set this value to their own.

sperand-io commented 8 years ago

I think it makes sense to generalize it per @vincentchio's suggestion and to use the default to set the option to seg in all cases where the consumer doesn't deliberately override it at initialization. With that in mind, this LGTM

f2prateek commented 8 years ago

cool sgtm