Provide a way to disable `DnsQueryLifecycleObserverFactory` #3552

ikhoon · 2021-05-12T06:56:13Z

Motivation:

Some users might not want to monitor DNS metrics
if a client sends requests to unspecified domains
such as webhook domains or redirect locations.
In that case, the cardinality of the metrics could be huge
because of various domain names.

Modifications:

Add disableDnsQueryLifecycleObserverFactory() option to DnsResolverGroupBuilder

Result:

You can now disable DNS metrics using DnsResolverGroupBuilder.disableDnsQueryMetrics().

ClientFactory.builder()
             .domainNameResolverCustomizer(builder -> {
                 builder.disableDnsQueryMetrics();
             })

Fixes Provide a way to disable the default DNS metrics #3368

Motivation: Some users might not want to monitor DNS metrics if a client sends requests to unspecified domains such as webhook domains or redirect locations. In that case, the cardinality of the metrics could be huge because of various domain names. Modifications: - Add `disableDnsQueryLifecycleObserverFactory()` option to `DnsResolverGroupBuilder` Result: - You can now disable DNS metrics using `DnsResolverGroupBuilder.disableDnsQueryLifecycleObserverFactory()`. ```java ClientFactory.builder() .domainNameResolverCustomizer(builder -> { builder.disableDnsQueryLifecycleObserverFactory(); }) ``` - Fixes line#3368

minwoox · 2021-05-12T07:27:24Z

core/src/main/java/com/linecorp/armeria/client/DnsResolverGroupBuilder.java

@@ -361,13 +376,17 @@ RefreshingAddressResolverGroup build(EventLoopGroup eventLoopGroup) {
                    new DefaultDnsQueryLifecycleObserverFactory(


observerFactory is not used when disableDnsQueryLifecycleObserverFactory is false so we should remove it. 😄

minwoox · 2021-05-12T07:29:39Z

core/src/main/java/com/linecorp/armeria/client/DnsResolverGroupBuilder.java

-                        new BiDnsQueryLifecycleObserverFactory(observerFactory,
-                                                               dnsQueryLifecycleObserverFactory));
+
+            if (!disableDnsQueryLifecycleObserverFactory) {


I guess there might be a chance that a user wants to set a customized DnsQueryLifecycleObserver while he/she doesn't want to use the DefaultDnsQueryLifecycleObserverFactory.
So how about changing the parameter to disableDnsQueryMetric and allow users to use the customized factory?

+1 Nice idea.

codecov · 2021-05-12T07:57:31Z

Codecov Report

Merging #3552 (f029e78) into master (3c89a1f) will increase coverage by 0.00%.
The diff coverage is 77.77%.

❗ Current head f029e78 differs from pull request most recent head 781a286. Consider uploading reports for the commit 781a286 to get more accurate results

@@            Coverage Diff             @@
##             master    #3552    +/-   ##
==========================================
  Coverage     73.92%   73.92%            
- Complexity    14210    14241    +31     
==========================================
  Files          1245     1248     +3     
  Lines         54193    54327   +134     
  Branches       6930     6938     +8     
==========================================
+ Hits          40060    40162   +102     
- Misses        10592    10627    +35     
+ Partials       3541     3538     -3

Impacted Files	Coverage Δ	Complexity Δ
...necorp/armeria/client/DnsResolverGroupBuilder.java	`56.00% <77.77%> (+2.80%)`	`22.00 <1.00> (+3.00)`
...rp/armeria/server/tomcat/ManagedTomcatService.java	`50.84% <0.00%> (-22.04%)`	`6.00% <0.00%> (-1.00%)`
...nternal/server/tomcat/Tomcat90ProtocolHandler.java	`50.00% <0.00%> (-15.39%)`	`11.00% <0.00%> (-4.00%)`
...rp/armeria/server/tomcat/TomcatServiceBuilder.java	`36.36% <0.00%> (-10.91%)`	`4.00% <0.00%> (-1.00%)`
...com/linecorp/armeria/common/DefaultRpcRequest.java	`65.30% <0.00%> (-8.51%)`	`15.00% <0.00%> (+2.00%)`	⬇️
...ecorp/armeria/common/stream/StreamMessageUtil.java	`76.66% <0.00%> (-6.67%)`	`11.00% <0.00%> (-1.00%)`
...p/armeria/common/stream/AbstractStreamMessage.java	`81.73% <0.00%> (-5.77%)`	`15.00% <0.00%> (ø%)`
...rmeria/common/stream/ConcatArrayStreamMessage.java	`75.82% <0.00%> (-2.20%)`	`9.00% <0.00%> (-1.00%)`
...uth/oauth2/OAuth2TokenIntrospectionAuthorizer.java	`63.15% <0.00%> (-1.76%)`	`11.00% <0.00%> (-1.00%)`
...inecorp/armeria/server/grpc/FramedGrpcService.java	`81.95% <0.00%> (-0.49%)`	`30.00% <0.00%> (ø%)`
... and 27 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 3c89a1f...781a286. Read the comment docs.

trustin · 2021-05-12T10:08:33Z

core/src/test/java/com/linecorp/armeria/client/DnsMetricsTest.java

+                Thread.sleep(1000);
+                assertThat(MoreMeters.measureAll(meterRegistry).entrySet().stream()
+                                     .anyMatch(entry -> entry.getKey()
+                                                             .contains("armeria.client.dns.queries")))


Suggested change

.contains("armeria.client.dns.queries")))

.startsWith("armeria.client.dns.")))

trustin · 2021-05-12T10:09:22Z

core/src/test/java/com/linecorp/armeria/client/DnsMetricsTest.java

+                        "armeria.client.dns.queries#count{" +
+                        "cause=others,name=bar.com.,result=failure}";
+                assertThat(MoreMeters.measureAll(meterRegistry))
+                        .doesNotContainKeys(writeMeterId, successMeterId);


How about making sure there are no meters that start with armeria.client.dns.?

trustin · 2021-05-12T10:10:03Z

core/src/main/java/com/linecorp/armeria/client/DnsResolverGroupBuilder.java

@@ -90,6 +90,7 @@
    private DnsServerAddressStreamProvider dnsServerAddressStreamProvider;
    @Nullable
    private DnsQueryLifecycleObserverFactory dnsQueryLifecycleObserverFactory;
+    private boolean disableDnsQueryMetric;


dnsQueryMetricsDisabled?

trustin · 2021-05-12T10:10:18Z

core/src/main/java/com/linecorp/armeria/client/DnsResolverGroupBuilder.java

+     * Disables the default {@link DnsQueryLifecycleObserverFactory} that collects DNS query metrics through
+     * {@link MeterRegistry}.
+     */
+    public DnsResolverGroupBuilder disableDnsQueryMetric() {


disableDnsQueryMetrics (s at the end)

minwoox

LGTM once @trustin's comments are addressed. 😄

…SimpleMeterRegistry

trustin

Thanks, @ikhoon !

ikhoon added the new feature label May 12, 2021

ikhoon added this to the 1.8.0 milestone May 12, 2021

ikhoon requested review from minwoox and trustin as code owners May 12, 2021 06:56

minwoox reviewed May 12, 2021

View reviewed changes

Address comments by @minwoox

f029e78

trustin reviewed May 12, 2021

View reviewed changes

minwoox approved these changes May 12, 2021

View reviewed changes

Address comments by @trustin, Replace PrometheusMeterRegistries with …

781a286

…SimpleMeterRegistry

trustin approved these changes May 14, 2021

View reviewed changes

trustin merged commit b5a2e1c into line:master May 14, 2021

ikhoon deleted the disable-dns-metrics-option branch May 27, 2021 01:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a way to disable `DnsQueryLifecycleObserverFactory` #3552

Provide a way to disable `DnsQueryLifecycleObserverFactory` #3552

ikhoon commented May 12, 2021 •

edited

Loading

minwoox May 12, 2021

minwoox May 12, 2021

ikhoon May 12, 2021

codecov bot commented May 12, 2021 •

edited

Loading

trustin May 12, 2021

trustin May 12, 2021

trustin May 12, 2021

trustin May 12, 2021

minwoox left a comment

trustin left a comment

		@@ -361,13 +376,17 @@ RefreshingAddressResolverGroup build(EventLoopGroup eventLoopGroup) {
		new DefaultDnsQueryLifecycleObserverFactory(

	.contains("armeria.client.dns.queries")))
	.startsWith("armeria.client.dns.")))

Provide a way to disable DnsQueryLifecycleObserverFactory #3552

Provide a way to disable DnsQueryLifecycleObserverFactory #3552

Conversation

ikhoon commented May 12, 2021 • edited Loading

minwoox May 12, 2021

Choose a reason for hiding this comment

minwoox May 12, 2021

Choose a reason for hiding this comment

ikhoon May 12, 2021

Choose a reason for hiding this comment

codecov bot commented May 12, 2021 • edited Loading

Codecov Report

trustin May 12, 2021

Choose a reason for hiding this comment

trustin May 12, 2021

Choose a reason for hiding this comment

trustin May 12, 2021

Choose a reason for hiding this comment

trustin May 12, 2021

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

Provide a way to disable `DnsQueryLifecycleObserverFactory` #3552

Provide a way to disable `DnsQueryLifecycleObserverFactory` #3552

ikhoon commented May 12, 2021 •

edited

Loading

codecov bot commented May 12, 2021 •

edited

Loading