Add MetricRecorder implementation #11128
Merged
Conversation
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
ejona86
reviewed
Apr 25, 2024
| // Measures may need updating in two cases: | ||
| // 1. When the sink is initially created with an empty list of measures. | ||
| // 2. When new metric instruments are registered, requiring the sink to accommodate them. | ||
| sink.updateMeasures(registry.getMetricInstruments()); |
There was a problem hiding this comment.
When you implement the sink, you'll need to check whether there are actually any new elements. With this threading, later calls to updateMeasures() can show fewer entries. That can be okay. The sink just needs to ignore the cases when there's no new entries.
core/src/test/java/io/grpc/internal/MetricRecorderImplTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/io/grpc/internal/MetricRecorderImplTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/io/grpc/internal/MetricRecorderImplTest.java
Outdated
Show resolved
Hide resolved
…addressed review comments
DNVindhya
changed the title
DNVindhya
commented
Apr 26, 2024
| * @param requiredLabelValues A list of required label values for the metric. | ||
| * @param optionalLabelValues A list of additional, optional label values for the metric. | ||
| */ | ||
| default void recordDoubleCounter(DoubleCounterMetricInstrument metricInstrument, double value, | ||
| default void addDoubleCounter(DoubleCounterMetricInstrument metricInstrument, double value, |
There was a problem hiding this comment.
@temawi updated counter APIs. Can you PTAL?
ejona86
approved these changes
Apr 26, 2024
| this(new ArrayList<>(DEFAULT_INSTRUMENT_LIST_CAPACITY), new HashSet<>()); | ||
| } | ||
| private volatile MetricInstrument[] metricInstruments; | ||
| private volatile int instrumentListCapacity = INITIAL_INSTRUMENT_CAPACITY; |
There was a problem hiding this comment.
Use metricInstruments.length instead
ejona86
approved these changes
Apr 26, 2024
| private final List<MetricInstrument> metricInstruments; | ||
| private final Set<String> registeredMetricNames; | ||
| private final Object lock = new Object(); | ||
| private final Set<String> registeredMetricNames = new HashSet<>(); |
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.
This PR adds an internal implementation for
MetricRecorderinterface. And adds unit tests forMetricInstrumentRegistry.