Use updated funnel schemas #4
Conversation
- Updated the client/server Dockerfiles to use Python 3.6/grpcio 1.4.0 - Added new TrackFunnel rpc to server - Generated code for client/server from new buda-protobufs schemas - Updated test code in client to use new schemas - Updated test code in client to send funnels and events
There was a problem hiding this comment.
Awesome to see the speed of the repo. Keep it flowing @michael-erasmus! 😄 As I said in our sync, looks good to me!
I left some unrelated comments where I'd like to hear your thoughts!
| @@ -1,4 +1,9 @@ | |||
| FROM grpc/python:1.0 | |||
| FROM python:3.6 | |||
There was a problem hiding this comment.
Nice and shiny Python 3.6 🙌
| @@ -5,7 +5,7 @@ all: run | |||
|
|
|||
| .PHONY: run | |||
| run: | |||
| docker run -d --net=host $(IMAGE_NAME) | |||
| docker run --net=host $(IMAGE_NAME) | |||
There was a problem hiding this comment.
Good catch! I forgot to remove the flag before pushing. Not sure why but the image wouldn't run in my PC if it wasn't as a daemon... 🤔
There was a problem hiding this comment.
Ah, I actually forgot I did that. I wanted to see the output for debugging, but I'm happy to revert to detached if that fixes your issue (I can always use docker log)
|
|
||
|
|
||
| class EventsCollectorServicer(object): | ||
| # missing associated documentation comment in .proto file |
There was a problem hiding this comment.
Found the code that generated this line and I think this is expected at the moment.
Adding comments before the service definition or rpc call will make them appear here instead of this comment. 😄
class EventsCollectorServicer(object):
"""This is only a test
"""
def CollectFunnelEvent(self, request, context):
"""Fancy comment
"""Would love to start thinking how we cat document protobufs!
There was a problem hiding this comment.
Nice find! Yeah, I think that would be an awesome best practice!
| @@ -27,13 +27,20 @@ def send_data_to_stream(self, data, stream_name): | |||
| Data=data, | |||
| PartitionKey='string' | |||
| ) | |||
| return response | |||
| return json.dumps(response) | |||
There was a problem hiding this comment.
Not related with this PR but wanted to start the convo. What do you think we should return to the gRPC server? I think we don't need to implement return codes as gRPC already handles them but we could add another layer of information in our response. I'm also happy to not return anything if everything worked properly.
There was a problem hiding this comment.
Good question, I'm happy to defer to your best judgment! Don't know what would be the most meaningful response?
I do think that most of the clients of the collector will probably more interested in a fire-and-forget flow and probably won't have a need to check the response unless there was an error (Even then I think most clients will probably silently catch the error, since we usually don't want to affect production because of a data collection error)
There was a problem hiding this comment.
Related though: I think it would be a good idea to still log the Kinesis response for debugging purposes (and maybe some other interesting data from the context as well?)
That's something we can do in Lambda right?
There was a problem hiding this comment.
I agree with you and I'm also not sure what to return 😊. I think we could return the Kinesis response at this moment and iterate it as needed.
That's something we can do in Lambda right?
Sorry, what would be the lambda for? Another thing to take into account is that CloudWatch Logs don't register the Lambda return value, only STDOUT.
Works in conjuction with bufferapp/buda-protobufs#4
Hey @davidgasquez. This PR is for the updated client/server code with the new funnel schemas
Brief summary of the changes: