1928 update name behavior #2302
Conversation
aneveu
commented
Aug 5, 2020
aneveu
commented
- remove the flow tag altogether
- using the name operator will replace the reactor prefix in meter names with whatever name was given to the sequence
- using tags but not name will be documented as potentially dangerous, as it creates a sparse set of tags for sequences in the application that don't have a unique name
- aggregated view is still somewhat possible in this mode by refraining from giving a name to the sequence, but consistently using a tag("flow", "theName") across all usages of .metrics() (which might be challenging if a framework or library also uses metrics() without a name())
Codecov Report
@@ Coverage Diff @@
## master #2302 +/- ##
============================================
+ Coverage 84.08% 84.11% +0.02%
- Complexity 4528 4530 +2
============================================
Files 378 378
Lines 31439 31440 +1
Branches 6035 6035
============================================
+ Hits 26436 26445 +9
+ Misses 3372 3367 -5
+ Partials 1631 1628 -3
Continue to review full report at Codecov.
|
docs/asciidoc/metrics.adoc
Outdated
| ==== | ||
|
|
||
| Be careful when using `Flux#name` and `Flux#tag` operators together: some monitoring systems like Prometheus require to have the exact same set of tags for each metric with the same name. |
There was a problem hiding this comment.
I'd turn this sentence around, indicating that using a name can be considered mandatory when using custom tags, since it would otherwise result in a different set of tags between two default-named sequences. then add the sentence about prometheus requiring exact same set of tags.
docs/asciidoc/metrics.adoc
Outdated
| .name("events") | ||
| .tag("source", "kafka") <1> |
There was a problem hiding this comment.
I'd also annotate the name line above
docs/asciidoc/metrics.adoc
Outdated
| === Common tags | ||
|
|
||
| Every metric will have the following tags in common: | ||
| Every metric will have the following tag in common: |
There was a problem hiding this comment.
since we only have one now, I'd eliminate the table entirely and inline the description of the tag in the sentence. Also list the two possible values, Flux and Mono.
| @@ -6001,12 +6001,16 @@ public int getPrefetch() { | |||
| * {@link #name(String) name} (and optionally {@link #tag(String, String) tag}) the | |||
| * sequence. | |||
| * <p> | |||
| * In case no name has been provided, the default name "reactor" will be applied. | |||
There was a problem hiding this comment.
Suggestion: add The name serves as a prefix in the reported metrics names. before that sentence
| @@ -6022,9 +6026,16 @@ public int getPrefetch() { | |||
| /** | |||
| * Give a name to this sequence, which can be retrieved using {@link Scannable#name()} | |||
| * as long as this is the first reachable {@link Scannable#parents()}. | |||
| * <p> | |||
| * If {@link #metrics()} operator is called later in the chain, this name will be used for | |||
| * {@link io.micrometer.core.instrument.Meter}. | |||
There was a problem hiding this comment.
don't reference micrometer classes in the javadoc. this can be turned into this name will be used as a prefix for metrics names (or meters instead of metrics)
| @@ -8289,10 +8300,17 @@ public final void subscribe(Subscriber<? super T> actual) { | |||
| * all tags throughout the publisher chain by using {@link Scannable#tags()} (as | |||
| * traversed | |||
| * by {@link Scannable#parents()}). | |||
| * <p> | |||
| * Note that some monitoring systems like Prometheus require to have the exact same set of | |||
| * tags for each {@link io.micrometer.core.instrument.Meter} bearing the same name. | |||
There was a problem hiding this comment.
don't link to micrometer classes in javadoc
| .summary(); | ||
|
|
||
| assertThat(meter).as("tagged find").isNull(); | ||
| } | ||
|
|
||
| //see https://github.com/reactor/reactor-core/issues/1425 |
There was a problem hiding this comment.
I'd still try to keep both of these tests, validating that a Mono.just and a Mono.error both have the same default set of tags. Just remove the .name (or make it the same). Same in the flux test.
docs/asciidoc/metrics.adoc
Outdated
|
|
||
| | type | Publisher's type | "Mono" | ||
| === Common tag |
There was a problem hiding this comment.
I wonder if, given the size of this section, it couldn't be merged with the Custom Tags section below.
