-
Notifications
You must be signed in to change notification settings - Fork 934
Add setMeterConfigurator support to MeterProvider
#7346
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
Merged
Merged
Changes from 31 commits
Commits
Show all changes
37 commits
Select commit
Hold shift + click to select a range
a202a21
swap handle async
fandreuz c83804d
static
fandreuz f4a4093
atomicref
fandreuz 7039b42
set meter config
fandreuz 0bd9d86
null checks
fandreuz 73a42da
generic drop agg
fandreuz 3e4ef5c
setEnabled
fandreuz 9136311
disable storages
fandreuz 6de7e98
ops
fandreuz b44530c
move down
fandreuz d9e39f1
unit tests
fandreuz c6c8f30
fix stuff and order
fandreuz b42e68a
Merge branch 'main' into 7051-metric-config
fandreuz 3b6f149
fix cmpl
fandreuz 285d43c
optimize setEnabled
fandreuz e656bd9
optimize setEnabled
fandreuz 21e188e
fix test name
fandreuz 03d159d
nonnnull
fandreuz bd141f6
new test
fandreuz b00d726
fix isEnabled
fandreuz fb846dc
new tests
fandreuz 30d81f5
new tests
fandreuz 21d36d5
memory mode
fandreuz 4196b17
cc
fandreuz 3c10154
aggregator reset tests
fandreuz 9b12918
more tests
fandreuz c8e0c85
Merge remote-tracking branch 'fandreuz/7051-metric-config' into 7051-…
fandreuz b21fa7b
ops
fandreuz e58c13c
nn
fandreuz d747d66
clear
fandreuz 6ca257d
no order
fandreuz 5f33bee
Update sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/interna…
fandreuz e5de344
Update sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/interna…
fandreuz 7ab9b3a
spotless
fandreuz f3e3b50
Simplify dynamic meter config mechanics
jack-berg f46b35e
Fix tests
jack-berg fbfc03c
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
jack-berg 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 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
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
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
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
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
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
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
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
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.
I suspect this is going to need some sort of memory barrier around it. Either make it an AtomicBoolean, or provide some kind of synchronization around the update to it.
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.
@jack-berg should this field be marked
volatile?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.
In other similar situations we've punted on the volatile keyword with the argument that even the small performance impact of reading a volatile field vs a non-volatile field is too high a price to pay the dynamism we get from this feature, which will only be leveraged by a few users. I know that in theory not marking it volatile may mean that the changes are never seen by readers, but I'd like to see complaints of that in practice before paying the performance penalty.
In this particular instance, adding volatile isn't terribly consequential because the field is only read on collection. The read work happens down in
DefaultSynchronousMetricStorage, which has anenabledfield which is also marked non-volatile and which is read each and every time a measurement is recorded. So we could updateSdkMeter#meterEnabledto be volatile, but I don't think we should makeDefaultSynchronousMetricStorage#enabledvolatile. So the question is, should we be markSdkMeter#meterEnabledbecause we can, even though its not really consequential? Or leave it be for consistency withSdkTracer,SdkLogger,DefaultSynchronousMetricStorage#enabled?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.
fair enough. I'm ok with it, if you're ok with it.