feat: Add new logger module - Pubstack Analytics Module #1331
Conversation
|
Let me know what I have to do this that PR ;) |
|
Sweet! The first analytics module! :) We'll review the PR tomorrow. |
|
🙏 Let me know how we can improve this first version/draft ;) |
|
Thank you for the first pass update. Heading in a good direction. The build passed on Go 1.12 and 1.13. It failed on 1.14 with the error: .. which looks like a failed http call. You'll want to stub out the http client to use canned responses in place of actual calls during the tests. My recommendation to allow configuration of the http client to avoid using the default client would help here. |
* V1 Pubstack (#7) * feat: Add Pubstack Logger (#6) * first version of pubstack analytics * bypass viperconfig * commit #1 * gofmt * update configuration and make the tests pass * add readme on how to configure the adapter and update the network calls * update logging and fix intake url definition * feat: Pubstack Analytics Connector * fixing go mod * fix: bad behaviour on appending path to auction url * add buffering * support bootstyrap like configuration * implement route for all the objects * supports termination signal handling for goroutines * move readme to the correct location * wording * enable configuration reload + add tests * fix logs messages * fix tests * fix log line * conclude merge * merge * update go mod Co-authored-by: Amaury Ravanel <[email protected]> * fix duplicated channel keys Co-authored-by: Amaury Ravanel <[email protected]> * first pass - PR reviews * rename channel* -> eventChannel * dead code * Review (#10) * use json.Decoder * update documentation * use nil instead []byte("") * clean code * do not use http.DefaultClient * fix race condition (need validation) * separate the sender and buffer logics * refactor the default configuration * remove error counter * Review GP + AR * updating default config * add more logs * remove alias fields in json * fix json serializer * close event channels Co-authored-by: Amaury Ravanel <[email protected]>
|
@SyntaxNode How can I re-enable travis? |
It's enabled, but it's been a bit flaky with reporting back to GitHub. I'll kick it off again. |
|
Hello, What's the next step here :)? |
Every code PR needs two approvals before being merged. I'm assigned as the second reviewer, but I've had a meeting heavy week. Sorry. I'm unintentionally holding you up here. I'll give it another pass today or worst case over the weekend. I'd really like to get this included in the next release. I greatly appreciate your responsiveness and support to the feedback. Building a module has a lot more complexity than an adapter. |
There was a problem hiding this comment.
Looking great! I'm very happy with how you addressed naming and potential thread race issues. I have a few comments for explicitly defining with the mux's are protecting, passing around the http client via the struct so it can be subbed out for tests and for a new pool in the near future (I'll do that part after this merges in), and missing test coverage. We strive for 90% test coverage and the packages in this PR are currently closer to 50%.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This reverts commit f57d9d6. # Conflicts: # analytics/config/config_test.go
|
Thanks gyus! (@hhhjort and @SyntaxNode) |
Add a new logger module: Pubstack
Ability for users to send bid events directly to Pubstack.