search

twmb / franz-go

franz-go contains a feature complete, pure Go library for interacting with Kafka from 0.8.0 through 3.7+. Producing, consuming, transacting, administrating, etc.
BSD 3-Clause "New" or "Revised" License
1.78k stars 182 forks source link

kotel: optionally only use messaging.kafka.connects.count for connection metrics #691

Closed endorama closed 3 months ago

endorama commented 6 months ago

This PR follows the discussion in https://github.com/twmb/franz-go/issues/670.

I added a new struct field, mergeConnectsMeter bool that is manipulated by WithMergedConnectsMeter(), to control whether to use one or 2 metrics to track connection successes/errors.

By default is set to false to retain current behaviour.

When set to true it disables the creation of messaging.kafka.connect_errors.count metric and follows a different path in Meter.OnBrokerConnect. There is some repetition as I wanted to avoid additional allocations when manipulating otel attributes (unfortunately Attribute Sets don't offer a way to add/remove elements from the set).

I added tests for the previous and new behaviour.

Let me know what you think, happy to adjust based on your feedback and thank you for your review!

endorama commented 6 months ago

@twmb I updated the PR to use attribute.Set and fixed the documentation. I also set this ready for review, happy to know what's your opinion!

endorama commented 4 months ago

@twmb as you've seen me moved away from relying on this monitoring plugin for the use case that spawn this contribution but I'm happy to follow along with it. I still think that it aligns better to expectations in the otel ecosystem so I see a benefit. Let me know if you need anything on my side here and thanks for your time!

twmb commented 4 months ago

Ah, sorry, mind dropping the go.mod and go.sum changes? I bumped them to latest before 1.17.

twmb commented 4 months ago

Pinging! @endorama I can clone your repo and edit your commit a bit, I'll give it another week or so but if you drop the go.mod and go.sum changes earlier, we can merge and release yours :)

endorama commented 4 months ago

@twmb sorry for the delay! I removed the commit bumping otel (bdbb6e1 (#691)) and rebased onto master so the branch is up to date.

Thanks for the review!