Mutiple metrics fixes #1930
Mutiple metrics fixes #1930
Conversation
dagnir
force-pushed
the
remove-metricutils
branch
from
5343501 to
90dbadd
Compare
| }); | ||
| return executeFuture; | ||
| } catch (Throwable t) { | ||
| Optional<MetricPublisher> metricPublisher = MetricUtils.resolvePublisher(clientConfiguration, | ||
| List<MetricPublisher> metricPublisher = resolveMetricPublishers(clientConfiguration, |
| @@ -1070,6 +1061,21 @@ public void close() { | |||
| .exceptionBuilderSupplier(InvalidInputException::builder).httpStatusCode(400).build()); | |||
| } | |||
|
|
|||
| private static List<MetricPublisher> resolveMetricPublishers(SdkClientConfiguration clientConfiguration, | |||
There was a problem hiding this comment.
Out of curiosity, why do we move this method out of MetricUtils?
There was a problem hiding this comment.
Discussed this with @millems. MetricUtils is @SdkInternal, and we'd rather keep it that way than expose it as a protected API.
| client = clientWithPublishers(publisher1, publisher2); | ||
|
|
||
| try { | ||
| client.allTypes().join(); |
There was a problem hiding this comment.
minor: missing fail("no exception thrown"), same as other tests.
There was a problem hiding this comment.
Is this necessary? It doesn't really matter if an exception is thrown or not; the test is just just checking that the right publishers are being used based on configuration. It just happens to throw here because the mock isn't fully set up
There was a problem hiding this comment.
oh okay, I misread the test. We don't need it in this case
dagnir
force-pushed
the
remove-metricutils
branch
3 times, most recently
from
f8875ae to
d0ec398
Compare
- Allow lists of publishers to be set. Allow setting of metric pubishers to be set on the client of request override rather than a single publisher, making using multiple MetricPublishers per client or request easier. - Remove any non-core usage of MetricUtils. - Fix metrics test in TranscribeStreamIntegrationTest. - Fix Async API call metric stage to add the duration in a separate CompletionStage.
dagnir
force-pushed
the
remove-metricutils
branch
from
d0ec398 to
db2a251
Compare
|
SonarCloud Quality Gate failed.
|
…f6b14e58c Pull request: release <- staging/992b5738-47e8-4c5c-a162-db9f6b14e58c
Description
Allow lists of publishers to be set.
Allow setting of metric pubishers to be set on the client of request override
rather than a single publisher, making using multiple MetricPublishers per
client or request easier.
Remove any non-core usage of MetricUtils.
Fix metrics test in TranscribeStreamIntegrationTest.
Motivation and Context
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsLicense