Upgrade tally to v4 (fixes uber-go/cadence-client#1157)

uber-go / cadence-client

Framework for authoring workflows and activities running on top of the Cadence orchestration engine.

https://cadenceworkflow.io

MIT License

344 stars 130 forks source link

Upgrade tally to v4 (fixes uber-go/cadence-client#1157) #1168

Closed nkcmr closed 2 years ago

nkcmr commented 2 years ago

What changed? Upgrade of the tally library to v4.

Why? Pre-v4 versions of tally used a non-official prometheus library (github.com/m3db/prometheus_client_golang) that causes integration issues with cadence users that need to feed cadence metrics into existing prometheus integrations that use the actual prometheus client library.

Tally v4 corrects this issue by using the official prometheus library.

How did you test it? This fork has been deployed to production systems at Cloudflare without much issue.

Potential risks The tally v4 upgrade does not appear to actually have contained that many "breaking" changes/behaviors, it just unforked prometheus client library and switch to go modules (https://github.com/uber-go/tally/releases/tag/v4.0.0). Risk seems minimal.

CLAassistant commented 2 years ago

All committers have signed the CLA.

nkcmr commented 2 years ago

Hey @vytautas-karpavicius, hate to bug you, but I saw you approving/commenting on other PRs and was hoping you could help. Is it possible to get some feedback on this PR from you or anyone on your team?

Worth noting that this fork is being used in our production systems without much issue if that helps. 🙂

Let me know if anything else is needed. Thanks so much.

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 01813f6d-55c4-48fe-b3bf-d89ef935eb11

0 of 0 changed or added relevant lines in 0 files are covered.
15 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.08%) to 63.741%

Files with Coverage Reduction	New Missed Lines	%
internal/internal_task_pollers.go	2	81.43%
internal/compatibility/thrift/enum.go	4	21.2%
internal/compatibility/thrift/types.go	9	48.42%
<!--	Total:	15	-->

Totals
Change from base Build 0181235a-93d0-4300-bc16-78a7c22ce7e1:	-0.08%
Covered Lines:	12383
Relevant Lines:	19427

💛 - Coveralls

vytautas-karpavicius commented 2 years ago

Hey @vytautas-karpavicius, hate to bug you, but I saw you approving/commenting on other PRs and was hoping you could help. Is it possible to get some feedback on this PR from you or anyone on your team?

Worth noting that this fork is being used in our production systems without much issue if that helps. 🙂

Let me know if anything else is needed. Thanks so much.

Thank you for the contribution!

nkcmr commented 2 years ago

@vytautas-karpavicius Thank you. Glad to help 😄

Groxx commented 2 years ago

hm :\ I'm not sure we can keep this, it's a breaking change since we expose tally.Scope in a number of locations in our API.