Add #1123 metrics to `Flux` #1183

simonbasle · 2018-05-01T21:42:58Z

work in progress first MVP

codecov-io · 2018-05-01T21:53:15Z

Codecov Report

Merging #1183 into master will increase coverage by 0.53%.
The diff coverage is 79.41%.

@@             Coverage Diff              @@
##             master    #1183      +/-   ##
============================================
+ Coverage     83.44%   83.98%   +0.53%     
- Complexity     3674     4496     +822     
============================================
  Files           342      349       +7     
  Lines         28206    29807    +1601     
  Branches       5285     5451     +166     
============================================
+ Hits          23537    25032    +1495     
- Misses         3077     3170      +93     
- Partials       1592     1605      +13

Impacted Files	Coverage Δ	Complexity Δ
.../main/java/reactor/core/publisher/MonoMetrics.java	`0% <0%> (ø)`	`0 <0> (?)`
...ore/src/main/java/reactor/core/publisher/Mono.java	`94.16% <16.66%> (-0.77%)`	`481 <1> (+236)`
...ore/src/main/java/reactor/core/publisher/Flux.java	`98.95% <37.5%> (-0.65%)`	`963 <2> (+459)`
...va/reactor/core/publisher/FluxMetricsFuseable.java	`76.92% <76.92%> (ø)`	`2 <2> (?)`
...va/reactor/core/publisher/MonoMetricsFuseable.java	`76.92% <76.92%> (ø)`	`2 <2> (?)`
.../main/java/reactor/core/publisher/FluxMetrics.java	`91.39% <91.39%> (ø)`	`7 <7> (?)`
...c/main/java/reactor/core/scheduler/Schedulers.java	`83.76% <0%> (-1.96%)`	`87% <0%> (+32%)`
...java/reactor/core/publisher/FluxBufferTimeout.java	`72.02% <0%> (-0.7%)`	`3% <0%> (ø)`
...eactor/core/publisher/ParallelMergeSequential.java	`79.79% <0%> (-0.52%)`	`7% <0%> (ø)`
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edfdd9e...57a11fd. Read the comment docs.

smaldini · 2018-05-07T19:39:26Z

Given how frequent or transverse this could be, we should add microfusion support as well.

smaldini · 2018-05-07T19:41:26Z

Should metrics for Schedulers be in a separate PR ?

simonbasle · 2018-05-09T07:38:47Z

Should metrics for Schedulers be in a separate PR ?

Yes I think it would make sense to consider this a MVP and fine tune meters and/or add instrumentation of schedulers in another PR

Given how frequent or transverse this could be, we should add microfusion support as well.

I'm on it 👍

In particular, the counters for error/complete/cancel are removed in favor of a tag on the subscribeToTermination timer, allowing for a drill-down-and-count approach. Tags (in the reactor sense) and sequence name are discovered at assembly time and injected as micrometer tags.

A Gauge with the same name and tags cannot wrap multiple AtomicLongs. A counter is more appropriate, but it cannot be decreased. It makes far less sense for a "generic" application-level point of view, so it is only activated when the Flux has been named.

broken: due to Gradle/IntelliJ integration, IntelliJ thinks micrometer is on all tests classpath as soon as it discovers metricsTest is a test sourceSet. On the other hand, if it doesn't recognize it as a test sourceSet, the code is marked as production which is also undesirable...

…sspath" This reverts commit f7b1124

The aim is to delay any explicit use of Micrometer types until subscription, so that the operator can still be instantiated even though Micrometer isn't on the classpath.

simonbasle · 2018-05-17T16:13:44Z

TODO:

make the Metrics class package-private (we don't expect most people to interact with meters programmatically, so they won't really need the constants)

smaldini force-pushed the master branch from 2849262 to 7bde523 Compare May 2, 2018 19:16

simonbasle force-pushed the 1123-metrics branch 2 times, most recently from cd7ecc7 to c0f332a Compare May 2, 2018 21:35

smaldini added the type/enhancement A general enhancement label May 5, 2018

smaldini added this to the 3.2.0.RELEASE milestone May 5, 2018

simonbasle force-pushed the 1123-metrics branch from 9c9a5f6 to 40505ee Compare May 7, 2018 16:51

simonbasle added 20 commits May 9, 2018 15:14

first go at #1123 Metrics

f4bdba2

for samples, pre-resolve the timers with appropriate tags. use builders

6cb6453

Use dot separator in meter and tag names, enabling normalization

bff87e2

add another baseUnit

be06236

Revert "wip Add test sourceSet and task for when Micrometer is in cla…

d71e875

…sspath" This reverts commit f7b1124

Use default sequenceName if scannable but name is stepName

99a495f

Remove no-op subscriber and just shortcircuit subscription

2bb5d6c

Don't measure last onNext to termination (so that count is relevant)

ce1f69a

Implement a few Metrics tests

5c2c3f0

Don't test tag aggregation, just test they're mapped to micrometer

50800b3

Directly use Timer.record and System.nanoTime on critical path (onNext)

1ab62c7

No need for storing common tags as field

427679a

Use DistributionSummary for request amount

fff9234

Add MonoMetrics + javadoc

95f26dc

Extract constants and library detection in reactor.util.Metrics

5925b7e

Cover all meters in FluxMetricsTest

a594dd7

Use Clock interface for time, could be useful later to mock time in test

8b56be9

simonbasle added 2 commits May 9, 2018 15:14

Mutualize name/tag extraction

7070cee

Add Fuseable Subscriber version that tracks onNext/onComplete/onError

fc19f3c

simonbasle force-pushed the 1123-metrics branch from dd6053e to fc19f3c Compare May 9, 2018 13:14

simonbasle added 3 commits May 9, 2018 16:40

Fuseable metrics operator for Mono and Flux, add fused tests

3e77c5f

Allow to pass the MeterRegistry in ctor, but as Object

3d9e435

The aim is to delay any explicit use of Micrometer types until subscription, so that the operator can still be instantiated even though Micrometer isn't on the classpath.

fix test to use termination type tag

f03e155

smaldini mentioned this pull request May 11, 2018

Add Scheduler Metrics support #1201

Closed

simonbasle modified the milestones: 3.2.0.RELEASE, 3.2.0.M2 May 17, 2018

simonbasle added 2 commits May 17, 2018 19:14

Guard all *Metrics usage with isMicrometerAvailable

33a688b

move all metric utils in FluxMetrics, package-private

57a11fd

simonbasle requested a review from smaldini May 17, 2018 17:33

smaldini approved these changes May 18, 2018

View reviewed changes

simonbasle merged commit 425bf5c into master May 18, 2018

simonbasle deleted the 1123-metrics branch May 18, 2018 16:27

simonbasle mentioned this pull request Aug 10, 2018

Provide base Metrics story from Scannable #1123

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add #1123 metrics to `Flux` #1183

Add #1123 metrics to `Flux` #1183

simonbasle commented May 1, 2018

codecov-io commented May 1, 2018 •

edited

Loading

smaldini commented May 7, 2018

smaldini commented May 7, 2018

simonbasle commented May 9, 2018 •

edited

Loading

simonbasle commented May 17, 2018 •

edited

Loading

Add #1123 metrics to Flux #1183

Add #1123 metrics to Flux #1183

Conversation

simonbasle commented May 1, 2018

codecov-io commented May 1, 2018 • edited Loading

Codecov Report

smaldini commented May 7, 2018

smaldini commented May 7, 2018

simonbasle commented May 9, 2018 • edited Loading

simonbasle commented May 17, 2018 • edited Loading

Add #1123 metrics to `Flux` #1183

Add #1123 metrics to `Flux` #1183

codecov-io commented May 1, 2018 •

edited

Loading

simonbasle commented May 9, 2018 •

edited

Loading

simonbasle commented May 17, 2018 •

edited

Loading