dashbitco / broadway_cloud_pub_sub

A Broadway producer for Google Cloud Pub/Sub
Apache License 2.0
70 stars 24 forks source link

Adds telemetry #90

Closed joladev closed 2 years ago

joladev commented 2 years ago

Adds span style events for pull request and ack, including tests and documentation.

Happy to take suggestions on naming. Potentially the existing demand is not something we should emit here? Added it cause it'd find it personally useful but happy to remove if it doesn't make sense.

It was a bit hard to do this because the config passed to ack doesn't come from the same place at all as the config passed to receive_messages, the first being just validated producer opts put in persistent term and not as you'd expect, the ack opts validated in the configure callback. And the second being the full config passed to the producer, including the broadway config. The test config doesn't really match the config when running for real, eg in the tests the receive_messages tests get the same config as the ack tests

josevalim commented 2 years ago

I think we should at least align the metadata naming with SQS. WDYT? https://hexdocs.pm/broadway_sqs/BroadwaySQS.Producer.html#module-telemetry

Aligning the event name would be good but not required. Thoughts?

joladev commented 2 years ago

I think we should at least align the metadata naming with SQS. WDYT? https://hexdocs.pm/broadway_sqs/BroadwaySQS.Producer.html#module-telemetry

Aligning the event name would be good but not required. Thoughts?

Ah of course, switching. What do you think about keeping :pull_client the way the RabbitMQ library has amqp?

joladev commented 2 years ago

Although looking at that, it only has demand (is that total demand or how many messages were requested?). In this case what's useful to expose is what the PullClient is setting as maxMessages. So maybe naming it max_messages?

joladev commented 2 years ago

Exposing messages seems unnecessary, since Broadway already does that

joladev commented 2 years ago

Made a suggested change, renaming total_demand -> demand, requested_messages -> max_messages. Added them to stop event.

Also renamed pull_request event name to receive_messages.

josevalim commented 2 years ago

:green_heart: :blue_heart: :purple_heart: :yellow_heart: :heart: