Skip to content
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

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

Merged
merged 4 commits into from
Jun 8, 2024

Conversation

endorama
Copy link
Contributor

@endorama endorama commented Mar 11, 2024

This PR follows the discussion in #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 endorama marked this pull request as ready for review March 25, 2024 10:52
@endorama
Copy link
Contributor Author

@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!

@twmb twmb mentioned this pull request Mar 26, 2024
12 tasks
@twmb twmb added the TODO label May 23, 2024
@endorama
Copy link
Contributor Author

@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 twmb added the plugin PRs related to plugins label May 26, 2024
twmb
twmb previously approved these changes May 26, 2024
Copy link
Owner

@twmb twmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I wanted to wait until I finally had time for 1.17 but really that wasn't necessary -- my bad. Anyway, 1.17 is now released, and now I'll release this one too, thank you!

@twmb
Copy link
Owner

twmb commented May 26, 2024

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

@twmb twmb added plugin PRs related to plugins minor and removed plugin PRs related to plugins TODO labels May 26, 2024
@twmb
Copy link
Owner

twmb commented Jun 4, 2024

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 added 4 commits June 5, 2024 19:47
Add metrics tests for both old and new behaviour
small change to align on using attribute.Set for metrics
attributes.
@endorama
Copy link
Contributor Author

endorama commented Jun 5, 2024

@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!

@twmb twmb merged commit a5b1d0c into twmb:master Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor plugin PRs related to plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants