WIP: new metrics emitter and histogram strategy #7201
Draft
+1,955
−94
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Groxx
commented
Aug 22, 2025
Comment on lines
228
to
233
| t.metrics.replicationLag( | ||
| t.ackLevels.UpdateIfNeededAndGetQueueMaxReadLevel(persistence.HistoryTaskCategoryReplication, pollingCluster), | ||
| taskInfos, | ||
| msgs, | ||
| t.scope, | ||
| ) |
There was a problem hiding this comment.
I've done it this way mostly because:
- the metrics-func is private and local to this file, so it's not too concerning to have it depend on excessively-large objects
- each argument is a unique type, so it's not possible to pass them wrong (the main alternative is to pass three integers).
Groxx
commented
Aug 22, 2025
common/metrics/structured/base.go
Outdated
Comment on lines
12
to
14
| var Module = fx.Provide(func(s tally.Scope) Emitter { | ||
| return Emitter{scope: s} | ||
| }) |
There was a problem hiding this comment.
this is the only constructor, outside tests. makes it fairly likely that I've got my fx / resource / context / etc injection correct.
Groxx
commented
Aug 22, 2025
Comment on lines
+51
to
+52
| // TODO: this can / likely should be turned into an interface to allow disconnecting from tally, | ||
| // to allow providing a specific version or to drop it entirely if desired. |
There was a problem hiding this comment.
I haven't built that here because the PR is already large, and it's a fair bit simpler to not introduce an interface.
but the resulting Emitter API is very simple compared to Tally, seems likely to be fairly easy to turn into an interface and build a different impl if needed.
Groxx
commented
Aug 22, 2025
| @@ -0,0 +1,476 @@ | |||
| package main | |||
There was a problem hiding this comment.
needs a moderate amount of cleanup / "report bad tag formats on the line of code that has the tag, not via a log.Fatal", but you get the idea.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proof of concept on display.
This is an attempt to move away from our opaque and broadly-disliked metrics system, and towards two major changes:
Once this runs in prod for a bit, so I have more data to play with, I'll build alerts and dashboards based on the new data, and we can check the runtime cost of these new metrics.
I've selected moderately-used and rather-expensive (high cardinality, high number of calls) metrics for this first one, to try to give us a realistic sample.