-
Notifications
You must be signed in to change notification settings - Fork 825
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
Batch Metrics Handler for Mutable State Metrics #6603
Open
njo
wants to merge
2
commits into
main
Choose a base branch
from
mutable_spans
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
// The MIT License | ||
// | ||
// Copyright (c) 2020 Temporal Technologies Inc. All rights reserved. | ||
// | ||
// Copyright (c) 2020 Uber Technologies, Inc. | ||
// | ||
// Permission is hereby granted, free of charge, to any person obtaining a copy | ||
// of this software and associated documentation files (the "Software"), to deal | ||
// in the Software without restriction, including without limitation the rights | ||
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell | ||
// copies of the Software, and to permit persons to whom the Software is | ||
// furnished to do so, subject to the following conditions: | ||
// | ||
// The above copyright notice and this permission notice shall be included in | ||
// all copies or substantial portions of the Software. | ||
// | ||
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
// THE SOFTWARE. | ||
|
||
//go:generate mockgen -copyright_file ../../LICENSE -package $GOPACKAGE -source $GOFILE -destination batchmetrics_mock.go | ||
|
||
package metrics | ||
|
||
import "time" | ||
|
||
// The BatchMetrics interfaces provide a way to emit "wide events" within the codebase. If a wide event | ||
// implementation is not provided we default to the implementation provided below which will maintain | ||
// backwards compatibility by emitting each field as an individual metric. In custom implementations, fields | ||
// can be labeled using the metricDefinition.Name() accessor within the respective With() methods. | ||
|
||
type ( | ||
BatchMetricsHandler interface { | ||
CreateBatch(string, ...Tag) BatchMetric | ||
} | ||
|
||
BatchMetric interface { | ||
WithHistogram(histogramDefinition, int64) BatchMetric | ||
WithTimer(timerDefinition, time.Duration) BatchMetric | ||
WithCounter(counterDefinition, int64) BatchMetric | ||
WithGauge(gaugeDefinition, float64) BatchMetric | ||
Emit() | ||
} | ||
|
||
BatchHandlerImpl struct { | ||
metricsHandler Handler | ||
} | ||
BatchMetricImpl struct { | ||
emitter Handler | ||
} | ||
) | ||
|
||
func NewBatchMetricsHandler(metricHandler Handler) BatchHandlerImpl { | ||
return BatchHandlerImpl{metricHandler} | ||
} | ||
|
||
func (bh BatchHandlerImpl) CreateBatch(_ string, tags ...Tag) BatchMetric { | ||
emitter := bh.metricsHandler | ||
if len(tags) > 0 { | ||
emitter = emitter.WithTags(tags...) | ||
} | ||
return &BatchMetricImpl{ | ||
emitter: emitter, | ||
} | ||
} | ||
|
||
func (bm *BatchMetricImpl) WithHistogram(def histogramDefinition, value int64) BatchMetric { | ||
def.With(bm.emitter).Record(value) | ||
return bm | ||
} | ||
|
||
func (bm *BatchMetricImpl) WithTimer(def timerDefinition, value time.Duration) BatchMetric { | ||
def.With(bm.emitter).Record(value) | ||
return bm | ||
} | ||
|
||
func (bm *BatchMetricImpl) WithCounter(def counterDefinition, value int64) BatchMetric { | ||
def.With(bm.emitter).Record(value) | ||
return bm | ||
} | ||
|
||
func (bm *BatchMetricImpl) WithGauge(def gaugeDefinition, value float64) BatchMetric { | ||
def.With(bm.emitter).Record(value) | ||
return bm | ||
} | ||
|
||
// Emit is a no-op in this implementation since we don't actually send wide events. | ||
func (bm *BatchMetricImpl) Emit() {} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains 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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already pretty annoying to pass around a bunch of these "context objects" like metrics.Handler and log.Logger (sometimes two loggers), so I hate to add more. Is there some way we could do this in the existing metrics.Handler? I guess we don't want to break existing implementations, so maybe the interface extension pattern? (though I know that has problems)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick look @dnr!
Sure, I agree plumbing this around the place is a pain. As you said, I don't want to break the existing interface so extending could work. Alternatively we could wrap the various observability handlers within some unified wrapper so we just have the one thing to pass around and we keep the batch handler distinct from the standard handler.
For the former I'd have BatchMetricsHandler extend metricsHandler and remove the basic metricsHandler from the context objects that were touched in this PR. Over time if and when other batch metrics are added in the codebase the batch handler can replace the old handler.
For the latter we'd have some kind of
ObservabilityHandler
containing the 3 types which replaces the Log / Metrics / BatchMetrics handlers in the relevant contexts. This would ultimately mean less things to pass around, the downside being the coupling of Logging with Metrics (though in practice, I'm not sure how much of an issue this would be).What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I discussed the approach with @njo originally, we had a different approach.
metrics.Handler
had aStartBatch()
method that returns aBatchHandler
, theBatchHandler
is just ametrics.Handler
and anio.Closer
.With that approach, you'd have:
The default implementation for a metrics handler would return itself with a nop closer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable @bergundy. I misunderstood when we talked that we'd be using the same metrics handler interface. To clarify, I'd be extending the existing interface here rather than creating a new Handler type that wraps the old one? So going through and updating our existing handler implementations and leaving folks with a custom handler to add the new batch method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have broken source-compatibility for users who use the server as a library before, but we try pretty hard not to. I feel like this case might be worth it, though, since it's very easy to fix. We should get a team consensus on that.
I like the ObservabilityHandler idea! We might need/want to do some unification of tags