Giveth / feathers-giveth

Featherjs server for caching giveth data.
MIT License
87 stars 99 forks source link

Improve Analytics Event Params #557

Closed divine-comedian closed 3 years ago

divine-comedian commented 3 years ago

To improve the analytics being sent we need to refine the information we're sending to segment from the track calls and to flatten nested properties into simple object names.

Another discussion is if we would like to include things like user email addresses and/or names which would make integration with any new email or user outreach systems easier to implement further down the road..definitely if we're integrating TRACE with .io we should be gathering similar information from users... input from @aminlatifi and @MoeNick would be appreciated

divine-comedian commented 3 years ago

More Context on flattening properties -

this is an example of a track call from the impact-graph on .io in the const segmentDonationReceived we define specifically the information we want and save it to an object with simple property names mapped to their value in the DB. We then put this object inside of the track call for the specific event we want to attach it to.

Save Donation


const projectOwner = await User.findOne({ id: Number(project.admin) })

        if (projectOwner) {

          await tokenValues

          analytics.identifyUser(projectOwner)

          const segmentDonationReceived = {
            email: projectOwner.email,
            title: project.title,
            firstName: projectOwner.firstName,
            projectOwnerId: project.admin,
            slug: project.slug,
            amount: Number(amount),
            transactionId: transactionId.toString().toLowerCase(),
            transactionNetworkId: Number(transactionNetworkId),
            currency: token,
            createdAt: new Date(),
            toWalletAddress: toAddress.toString().toLowerCase(),
            fromWalletAddress: fromAddress.toString().toLowerCase(),
            donationValueUsd: donation.valueUsd,
            donationValueEth: donation.valueEth
          }

          analytics.track(
            'Donation received',
            projectOwner.segmentUserId(),
            segmentDonationReceived,
            projectOwner.segmentUserId()
            )
        }

we specify all the information we want to capture and assign an object with flattened property names that get the information we want. It should come out clean on the segment side like this:

analytics.track({
  anonymousId: 'givethId-161',
  userId: 'givethId-161',
  event: 'Donation received',
  properties: {
    amount: 0.001,
    createdAt: new Date('2021-07-08T12:41:36.526Z'),
    currency: 'ETH',
    donationValueEth: 0.001,
    donationValueUsd: 2.17127,
    email: 'mrnikkhah@gmail.com',
    firstName: 'Moe',
    fromWalletAddress: '0x62f49a875f09c82e104197afbb8a30e66bb96ba3',
    projectOwnerId: '161',
    slug: 'myfirstprojectonio',
    title: 'MyFirstProjectOnIO',
    toWalletAddress: '0xf3bab2f1add2af035f489d7f3fb45a2411ed89ba',
    transactionId: '0x0ddd1a41af785ea73a54073a669456303adafba99f53183324fa2911043b656c',
    transactionNetworkId: 3
  }
});
divine-comedian commented 3 years ago

Here's a list of properties I think we should include for each event on TRACE, if anything is unclear, incomplete or inconsistent please reach out @mohammadranjbarz

@aminlatifi @MoeNick add emails to all calls? add names to all calls?

mohammadranjbarz commented 3 years ago

@MoeNick I think we should move this issue to sprint backlog and start implementing, am I right?

MoeNick commented 3 years ago

@mohammadranjbarz We would better make a copy and new estimates...

mohammadranjbarz commented 3 years ago

@mohammadranjbarz We would better make a copy and new estimates...

It's new issue, (Mitch has created this yesterday) I put the estimate

mohammadranjbarz commented 3 years ago

@divine-comedian Now you can test the changes on event params in here: https://deploy-preview-2399--giveth-dapp.netlify.app (or you can run #2399 locally) the changes that you wanted almost done except some items: somewhere you said the parentCampaignAddress but I didn't understand what you mean, do you mean campaignLink in giveth-dapp? or for example reviewerId, for users we have address and the reviewerAddress is enough ( actually we don't use id field for user never). or somewhere you wanted campaignSlug but at that point that we fire event we just have id of campaign, for getting slug we should send another request to get the campaign slug that I think it doesn't worth to do,

Anyway please, test it and say to me if there is something missing

Screen Shot 1400-04-27 at 23 44 21
divine-comedian commented 3 years ago

@mohammadranjbarz parentCampaignAddress would be the wallet address of the parent Campaign reviewerID would be so we can alias if the reviewer is the same user somewhere else on TRACE, assuming the user ID is the same for any entity or role they become... campaignSlug will be important to have for if we later want to integrate with autopilot.

I'll start testing now!

mohammadranjbarz commented 3 years ago

@divine-comedian Thanks mitch, the changes are on UAT, campaigns have wallet address? I should talk about it with you in DM

divine-comedian commented 3 years ago

There it is! I added checkboxes to all the things that need to be reviewed and fixed! let me know if you see anything else that I missed or isn't clear!

mohammadranjbarz commented 3 years ago

@divine-comedian Thanks for testing, before deploying on UAT you can test my changes here https://deploy-preview-2410--giveth-dapp.netlify.app

I have some question: what is entityAddress? because we entityOwnerAddress, and entityId so what should we do whats is parentEntityAddress while we have parentEntityId and parentEntityOwnerAddress what should be entityOwnerId when we have entityOwnerAddress what is traceOwnerId when we have traceOwnerAddress

MoeNick commented 3 years ago

@divine-comedian Hey Mitch, you can do the UAT I think. Lots of thanks.

divine-comedian commented 3 years ago

Had a follow up call with mohammad today, I will have time on wednesday hopefully to begin testing these fixes.

divine-comedian commented 3 years ago

Okay! Tested again all fixes, here's the final review:

Donation to Community

Trace Withdraw

Campaign Created

Trace Create - good

Trace Accepted - too much info being sent , trace object being sent -

image

Trace Rejected - no event showed up in segment

Trace Cancelled - good

Trace Approved - Good

Donation to Trace (All donation events) -

Community Created - good

Delegated Community to Campaign - change parentEntityOwnerId to parentEntityOwnerAddress

Delegated Community to Trace - change parentEntityOwnerId to parentEntityOwnerAddress

Delegated Campaign to Trace - change parentEntityOwnerId to parentEntityOwnerAddress

Donate to Campaign - good

Campaign Cancelled - good

mohammadranjbarz commented 3 years ago

Okay! Tested again all fixes, here's the final review:

Donation to Community

  • inconsistent naming from usdValue to valueEth

Trace Withdraw

  • missing traceType

Campaign Created

  • same address is captured for campaignAddress, ownerAddress and userAddress - consolidate to campaignOwnerAddress and userAddress

Trace Create - good

Trace Accepted - too much info being sent , trace object being sent -

image

Trace Rejected - no event showed up in segment

Trace Cancelled - good

Trace Approved - Good

Donation to Trace (All donation events) -

  • inconsistent naming from usdValue to valueEth

Community Created - good

Delegated Community to Campaign - change parentEntityOwnerId to parentEntityOwnerAddress

Delegated Community to Trace - change parentEntityOwnerId to parentEntityOwnerAddress

Delegated Campaign to Trace - change parentEntityOwnerId to parentEntityOwnerAddress

Donate to Campaign - good

Campaign Cancelled - good

@divine-comedian Thanks Mitch.

@MoeNick As we talked before We should create a new task for fixing these items that Mitch mentioned, am I right?

MoeNick commented 3 years ago

@mohammadranjbarz Of course. I'll do it.

divine-comedian commented 3 years ago

Are there any updates on this issue? Does it need further testing or is it ready to deploy?

MoeNick commented 3 years ago

We dont plan for remained items in currant sprint cause we were full, will start it again from Tue, our next sprint.

mohammadranjbarz commented 3 years ago

Okay! Tested again all fixes, here's the final review:

Donation to Community

  • inconsistent naming from usdValue to valueEth

Trace Withdraw

  • missing traceType

Campaign Created

  • same address is captured for campaignAddress, ownerAddress and userAddress - consolidate to campaignOwnerAddress and userAddress

Trace Create - good

Trace Accepted - too much info being sent , trace object being sent -

image

Trace Rejected - no event showed up in segment

Trace Cancelled - good

Trace Approved - Good

Donation to Trace (All donation events) -

  • inconsistent naming from usdValue to valueEth

Community Created - good

Delegated Community to Campaign - change parentEntityOwnerId to parentEntityOwnerAddress

Delegated Community to Trace - change parentEntityOwnerId to parentEntityOwnerAddress

Delegated Campaign to Trace - change parentEntityOwnerId to parentEntityOwnerAddress

Donate to Campaign - good

Campaign Cancelled - good

Thanks to Mitch, these are the works I should do for this task

mohammadranjbarz commented 3 years ago

Okay! Tested again all fixes, here's the final review: Donation to Community

  • inconsistent naming from usdValue to valueEth

Trace Withdraw

  • missing traceType

Campaign Created

  • same address is captured for campaignAddress, ownerAddress and userAddress - consolidate to campaignOwnerAddress and userAddress

Trace Create - good Trace Accepted - too much info being sent , trace object being sent - image Trace Rejected - no event showed up in segment Trace Cancelled - good Trace Approved - Good Donation to Trace (All donation events) -

  • inconsistent naming from usdValue to valueEth

Community Created - good Delegated Community to Campaign - change parentEntityOwnerId to parentEntityOwnerAddress Delegated Community to Trace - change parentEntityOwnerId to parentEntityOwnerAddress Delegated Campaign to Trace - change parentEntityOwnerId to parentEntityOwnerAddress Donate to Campaign - good Campaign Cancelled - good

Thanks to Mitch, these are the works I should do for this task

  • [x] Trace Rejected - no event showed up in segment
  • [x] Trace Accepted - too much info being sent , trace object being sent
  • [x] Inconsistent naming from usdValue to valueEth (changing valueEth to ethValue)
  • [x] Delegated Community to Campaign - change parentEntityOwnerId to parentEntityOwnerAddress
  • [x] Delegated Community to Trace - change parentEntityOwnerId to parentEntityOwnerAddress
  • [x] Delegated Campaign to Trace - change parentEntityOwnerId to parentEntityOwnerAddress

@divine-comedian All items implemented, you can test those here: https://develop.giveth.io

divine-comedian commented 3 years ago

Nice work @mohammadranjbarz I tested all the issues you mentioned above. I found only one inconsistency with the Trace Rejected event. It is missing some properties compared to the Trace Accepted event:

ownerAddress: 
parentCampaignAddress: 
parentCampaignTitle: 
slug: 
traceOwnerAddress: 
traceOwnerName: 
txUrl: 
mohammadranjbarz commented 3 years ago

Nice work @mohammadranjbarz I tested all the issues you mentioned above. I found only one inconsistency with the Trace Rejected event. It is missing some properties compared to the Trace Accepted event:

ownerAddress: 
parentCampaignAddress: 
parentCampaignTitle: 
slug: 
traceOwnerAddress: 
traceOwnerName: 
txUrl: 

@divine-comedian You are right, I added missing paramteres, you can check it here https://deploy-preview-2483--giveth-dapp.netlify.app

PS: in proposing and rejecting traces all things happen in our DB, not blockchain, so we don't have any txUrl to send ( in accept is different there we have txUrl because after accepting we send trace to blockchain network)

divine-comedian commented 3 years ago

good! just tested it, everything looks perfect.

image

mohammadranjbarz commented 3 years ago

@divine-comedian Thanks man,

@MoeNick I think you can move this issue to DONE