-
Notifications
You must be signed in to change notification settings - Fork 0
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
Updated funnel schemas #4
Conversation
- Seperated funnels and funnel events - Added a TrackFunnel rpc - Broke out custom uuid message for id's - Brought funnel and funnel event schemas in line with current schema - Moved funnel tags to be a map and remove the FunnelTag message - Created a Link schema for generic data linking
buda/entities/uuid.proto
Outdated
|
||
package buda.entities; | ||
|
||
message UUID { |
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 like this approach! 😄 I did a bit of 🔍 and seems there's an open issue to represent UUIDs in protobufs.
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.
Kinda a more general question but, how do you think we could name messages like UTM
s or UUID
s. For UTMs I used their style guide recommendationsbut I wasn't 100% sure if that was a good fit.
Looking at their go Uuid
, seems it's CamelCase but happy to use any style and stick with it!
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.
Looking at their go Uuid, seems it's CamelCase but happy to use any style and stick with it!
Good one @davidgasquez! To be honest, I glossed over the naming a bit and didn't give it much thought.
Happy to use Uuid
instead!
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.
Thanks for the PR @michael-erasmus! I left a couple of comments in the code but looks good to me. 😄
As I said in our sync, feel free to change anything in the repo, including the folder structure if this one doesn't make sense or you think it could be simplified!
buda/entities/uuid.proto
Outdated
|
||
package buda.entities; | ||
|
||
message UUID { |
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.
Kinda a more general question but, how do you think we could name messages like UTM
s or UUID
s. For UTMs I used their style guide recommendationsbut I wasn't 100% sure if that was a good fit.
Looking at their go Uuid
, seems it's CamelCase but happy to use any style and stick with it!
|
||
import "buda/entities/uuid.proto"; | ||
|
||
message Link { |
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.
Curious if you see Link being used in other entities? Thinking it could be moved inside funnel_event.proto
if not! 😊
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.
@davidgasquez Yeah, was kind of on the fence on that, but I thought 'maybe' it could be used by other entities. Reflecting on it now, I'm thinking perhaps I should stick to the current use 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.
@davidgasquez still can't make up my mind on this one :D Maybe I could keep things as is (still want to validate if the whole Links idea is worth it) and come back to it later (before we go 'live')
Hi @davidgasquez! As we discussed, here is the PR with the new funnel and funnel events protobuf and gRPC service schema's
Brief summary of the changes: