Implementing dropwizard emitter for druid #7363

nishantmonu51 · 2019-03-27T22:54:05Z

This is an update for previous PR - #4196
Corresponding issues are
#3927
#1996

This extension integrates Dropwizard metrics library with druid so that dropwizard users can easily absorb druid into their monitoring ecosystem. It accumulates druid metrics as dropwizard metrics, and emits them to various sinks via dropwizard supported reporters.

Currently supported dropwizard metrics types counter, gauge, meter, timer and histogram.
These metrics can be emitted using either Console or JMX reporter.

Support for other reporters can be easily added in future PRs.

Note - I have cherry-picked the commit from previous author to provide him the credit of his work.

nishantmonu51 · 2019-04-22T15:14:57Z

@jihoonson @b-slim: Can you please help with the review here ?
It will be a nice addition to 0.15 release.

b-slim · 2019-04-25T16:52:22Z

docs/content/development/extensions-contrib/dropwizard.md

+## Introduction
+
+This extension integrates [Dropwizard](http://metrics.dropwizard.io/3.1.0/getting-started/#) metrics library with druid so that dropwizard users can easily absorb druid into their monitoring ecosystem.
+It accumulates druid metrics as dropwizard metrics, and emits them to various sinks via dropwizard supported reporters.


this is scary, how much it accumulates is this onheap or offheap? is there a config to bound this ?

the accumulation does not mean it queues things up, it stores the latest values in the metrics registry, the memory usage depends on the number of metrics and metric types.

Note: We are not handling all individual query metrics here, it's just the metrics defined in the extension config which are counters, gauges and timers. So have minimal impact on heap usage.e.g. for a counter it uses a single AtomicLong, for gauge it gives an instantaneous value and we are caching that in the metrics extensions impl.

https://metrics.dropwizard.io/4.0.0/getting-started.html has more details on the individual event types.

b-slim · 2019-04-25T16:54:20Z

docs/content/development/extensions-contrib/dropwizard.md

+|--------|-----------|---------|-------|
+|`druid.emitter.dropwizard.reporters`|List of dropwizard reporters to be used.|yes|none|
+|`druid.emitter.dropwizard.prefix`|Optional prefix to be used for metrics name|no|none|
+|`druid.emitter.dropwizard.includeHost`|Flag to include the hostname as part of the metric name.|no|yes|


does this includes host and ports ? or only host?

host and port both, It is essentially ServiceMetricEvent.getHost() value

Would you please make the doc more clear that it includes both host and port?

b-slim · 2019-04-25T16:55:20Z

docs/content/development/extensions-contrib/dropwizard.md

+|`druid.emitter.dropwizard.reporters`|List of dropwizard reporters to be used.|yes|none|
+|`druid.emitter.dropwizard.prefix`|Optional prefix to be used for metrics name|no|none|
+|`druid.emitter.dropwizard.includeHost`|Flag to include the hostname as part of the metric name.|no|yes|
+|`druid.emitter.dropwizard.includeDimensionNames`|Flag to include the dimension names as part of the metric name.|no|yes|


am trying to understand here, why a used would like to get a metric without dimension name? am not sure am getting this.

it is mostly a copy of other stats-d emitter extension and left over of that, Agree, It is not much useful for dropwizard, will remove

b-slim · 2019-04-25T16:56:03Z

docs/content/development/extensions-contrib/dropwizard.md

+|`druid.emitter.dropwizard.prefix`|Optional prefix to be used for metrics name|no|none|
+|`druid.emitter.dropwizard.includeHost`|Flag to include the hostname as part of the metric name.|no|yes|
+|`druid.emitter.dropwizard.includeDimensionNames`|Flag to include the dimension names as part of the metric name.|no|yes|
+|`druid.emitter.dropwizard.dimensionMapPath`|Path to JSON file defining the StatsD type, and desired dimensions for every Druid metric|no|Default mapping provided. See below.|


is statsD a typo mistake or it is actually called statsD ?

left over of copy and paste, fixed.

b-slim · 2019-04-25T16:57:15Z

docs/content/development/extensions-contrib/dropwizard.md

+
+Each metric emitted using Dropwizard must specify a type, one of `[timer, counter, guage, meter, histogram, ]`. Dropwizard Emitter expects this mapping to
+be provided as a JSON file.  Additionally, this mapping specifies which dimensions should be included for each metric.
+If the user does not specify their own JSON file, a default mapping is used.


can we add a link to the default mapping ?

b-slim · 2019-04-25T16:58:12Z

docs/content/development/extensions-contrib/dropwizard.md

+Each metric emitted using Dropwizard must specify a type, one of `[timer, counter, guage, meter, histogram, ]`. Dropwizard Emitter expects this mapping to
+be provided as a JSON file.  Additionally, this mapping specifies which dimensions should be included for each metric.
+If the user does not specify their own JSON file, a default mapping is used.
+All metrics are expected to be mapped. Metrics which are not mapped will log an error.


how about ignored or warning ? seems like error is too much, especially in Druid we add metrics pretty often.

changed to ignored.

b-slim · 2019-04-25T19:33:38Z

...ropwizard-emitter/src/main/java/org/apache/druid/emitter/dropwizard/DropwizardConverter.java

+    metricMap = readMap(mapper, dimensionMapPath);
+  }
+
+  @Nullable


can you please add docs on how and why this will return null ? plus i see this nullable annotation all over the Druid code, am not sure what is used for?

added javadoc. null is returned when there is no mapping for a metric.

b-slim · 2019-04-25T19:35:32Z

...ropwizard-emitter/src/main/java/org/apache/druid/emitter/dropwizard/DropwizardConverter.java

+      String service,
+      String metric,
+      Map<String, Object> userDims,
+      Map<String, String> builder


reading the signature of this method i can not tell what is actually doing and why a map object is called a builder ?

changed the builder to filteredDimensions and mentioned it in javadoc.

b-slim · 2019-04-25T19:36:53Z

...ropwizard-emitter/src/main/java/org/apache/druid/emitter/dropwizard/DropwizardConverter.java

+        This is because some metrics are reported differently, but with the same name, from different services.
+       */
+    DropwizardMetricSpec metricSpec = null;
+    if (metricMap.containsKey(metric)) {


can we do this in one get and a null check?

b-slim · 2019-04-25T19:37:13Z

...ropwizard-emitter/src/main/java/org/apache/druid/emitter/dropwizard/DropwizardConverter.java

+      }
+      return metricSpec;
+    } else {
+      return null;


what is the meaning of null ?

it means no mapping found for metric

b-slim · 2019-04-25T19:43:06Z

...ropwizard-emitter/src/main/java/org/apache/druid/emitter/dropwizard/DropwizardConverter.java

+      {
+      }).readValue(is);
+    }
+    catch (IOException e) {


how about closing the stream on exception ?

Hmm, why is is not closed on normal exit?

thought ObjectReader.readValue will close the stream in normal flow

ObjectReader.readValue(..) doesn't mention closing the InputStream by itself, it might be safer to close it in finally block.

b-slim · 2019-04-25T19:43:50Z

.../dropwizard-emitter/src/main/java/org/apache/druid/emitter/dropwizard/DropwizardEmitter.java

+
+
+  @Override
+  public void start()


not sure why we are not guarding this and check for start not set ?

added guards.

Hmm, when can it be called twice? It should be managed by the lifecycle.

yeah, it should be called through lifecycle, It seems other emitter has guards too so i added a it here also.

LifecycleLock utility is useful for this usecase

b-slim · 2019-04-25T20:00:03Z

...ropwizard-emitter/src/main/java/org/apache/druid/emitter/dropwizard/DropwizardConverter.java

+public class DropwizardConverter
+{
+  private static final Logger log = new Logger(DropwizardConverter.class);
+  private Map<String, DropwizardMetricSpec> metricMap;


b-slim · 2019-04-25T20:00:26Z

.../dropwizard-emitter/src/main/java/org/apache/druid/emitter/dropwizard/DropwizardEmitter.java

+
+public class DropwizardEmitter implements Emitter
+{
+  private static Logger log = new Logger(DropwizardEmitter.class);


private static final

b-slim · 2019-04-25T20:10:21Z

.../dropwizard-emitter/src/main/java/org/apache/druid/emitter/dropwizard/DropwizardEmitter.java

+
+  @Override
+  public void close()
+  {


same as for start, shouldn't we check for start is true ?

added guard

b-slim · 2019-04-25T20:10:55Z

.../dropwizard-emitter/src/main/java/org/apache/druid/emitter/dropwizard/DropwizardEmitter.java

+  public void flush()
+  {
+    for (DropwizardReporter reporter : reporters) {
+      reporter.flush();


can we make sure this is not blocking?

yes, it is non-blocking, basically a no-op for the current two implementations of reporters.

we could mention that it must be non blocking in the javadoc of DropwizardReporter.flush(), so that if someone implements another then they know about it.

b-slim · 2019-04-25T20:16:48Z

...izard-emitter/src/main/java/org/apache/druid/emitter/dropwizard/DropwizardEmitterModule.java

+      final Injector injector
+  )
+  {
+    List<Emitter> alertEmitters = Lists.transform(


can we use native JDK functions/streams and move away from guava.

b-slim · 2019-04-25T20:20:38Z

...itter/src/main/java/org/apache/druid/emitter/dropwizard/reporters/DropwizardJMXReporter.java

+  @Override
+  public void close()
+  {
+


nothing to close ? this reporter has not state to clean ?

b-slim · 2019-04-25T20:23:12Z

extensions-contrib/graphite-emitter/pom.xml

-  ~ KIND, either express or implied.  See the License for the
-  ~ specific language governing permissions and limitations
-  ~ under the License.
+ ~ Licensed to Metamarkets Group Inc. (Metamarkets) under one


not sure why this change is needed ?

b-slim · 2019-04-25T20:23:27Z

indexing-service/src/main/java/org/apache/druid/indexing/overlord/RemoteTaskRunner.java

@@ -126,6 +126,9 @@
 * <p>
 * The RemoteTaskRunner uses ZK for job management and assignment and http for IPC messages.
 */
+
+
+              //KubernetesTaskRunner


b-slim · 2019-04-25T20:43:51Z

@nishantmonu51 am very ignorant of this DropWizard stuff, can you please explain where the metrics are buffered ? and how often cleaned ? how we can make sure it does not overflow ? and how the JMX one works do we need some Jvm flags to be set to turn it on can you please document how we can test that ?

b-slim · 2019-04-26T17:55:43Z

.../dropwizard-emitter/src/main/java/org/apache/druid/emitter/dropwizard/DropwizardEmitter.java

+  private final DropwizardEmitterConfig config;
+  // Note: the gauges do not represent the actual instantaneous value for the metrics.
+  // Instead they have the last known value for it.
+  private final ConcurrentHashMap<String, Number> gagues = new ConcurrentHashMap<>();


so if let say we have a metric where the set of the key grows over time eg queryId or something similar, this map will grow without any bound and that can lead to big troubles, how about making the keys weak refs or having an actual limit on the size of this map ?

looking into it, we can add a limit to the map size.

added a limit to the size of map.

stale · 2019-06-25T18:54:16Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions.

stale · 2019-06-26T16:26:56Z

This pull request/issue is no longer marked as stale.

stale · 2019-06-26T16:26:57Z

This pull request/issue is no longer marked as stale.

nishantmonu51 · 2019-09-02T07:26:48Z

@jihoonson @b-slim : Updated PR based on review comments, please check.

himanshug · 2019-09-04T23:12:31Z

docs/design/extensions-contrib/dropwizard.md

+Used to report druid metrics via JMX.
+```
+
+druid.emitter.dropwizard.reporter={"type":"jmx"}


Suggested change

druid.emitter.dropwizard.reporter={"type":"jmx"}

druid.emitter.dropwizard.reporters=[{"type":"jmx"}]

himanshug · 2019-09-04T23:12:50Z

docs/design/extensions-contrib/dropwizard.md

+
+```
+
+druid.emitter.dropwizard.reporter={"type":"console","emitIntervalInSecs":30}"}


Suggested change

druid.emitter.dropwizard.reporter={"type":"console","emitIntervalInSecs":30}"}

druid.emitter.dropwizard.reporters=[{"type":"console","emitIntervalInSecs":30}"}]

himanshug · 2019-09-04T23:13:45Z

docs/design/extensions-contrib/dropwizard.md

+```
+
+### Default Metrics Mapping
+```json


I wonder if it is possible to point this to actual file in the source code because this has the potential to go out of sync

added a link too. Initially it was a link only but added the file as part of review comments.

himanshug · 2019-09-04T23:21:57Z

.../dropwizard-emitter/src/main/java/org/apache/druid/emitter/dropwizard/DropwizardEmitter.java

+          nameBuilder.add(config.getPrefix());
+        }
+        nameBuilder.add("metric=" + metric);
+        nameBuilder.add("service=" + service);


should use string format instead of concatenations here and in all other places as this code is gonna be called many many times.

himanshug · 2019-09-04T23:27:47Z

...izard-emitter/src/main/java/org/apache/druid/emitter/dropwizard/DropwizardEmitterConfig.java

+      @JsonProperty("maxGaugeCount") Integer maxGaugeCount
+  )
+  {
+    this.reporters = reporters;


null/empty check ? it would be useless to set it up with no reporters and probably a configuration error.

himanshug · 2019-09-04T23:32:45Z

.../dropwizard-emitter/src/main/java/org/apache/druid/emitter/dropwizard/DropwizardEmitter.java

+  private final DropwizardEmitterConfig config;
+  // Note: the gauges do not represent the actual instantaneous value for the metrics.
+  // Instead they have the last known value for it.
+  private final Map<String, Number> gagues;


Suggested change

private final Map<String, Number> gagues;

private final Map<String, Number> gauges;

himanshug · 2019-09-04T23:36:02Z

.../dropwizard-emitter/src/main/java/org/apache/druid/emitter/dropwizard/DropwizardEmitter.java

+    this.config = config;
+    this.reporters = config.getReporters();
+    this.converter = new DropwizardConverter(mapper, config.getDimensionMapPath());
+    this.gagues = Collections.synchronizedMap(new GaugesCache<>(config.getMaxGaugeCount()));


could we use guava Cache instead of doing our own ?

done. replaced it use caffeine cache instead.

making metric manager and alert emitters as optional

more improvements improve docs refactrings

review comments review comments

himanshug · 2019-09-24T23:09:05Z

LGTM overall, can you please address
https://github.com/apache/incubator-druid/pull/7363/files#r321013974
https://github.com/apache/incubator-druid/pull/7363/files#r321015861 ?

himanshug

please fix the build

nishantmonu51 mentioned this pull request Mar 27, 2019

Implementing dropwizard emitter for druid (fixes #3927) #4196

Closed

b-slim reviewed Apr 25, 2019

View reviewed changes

b-slim reviewed Apr 26, 2019

View reviewed changes

stale bot added the stale label Jun 25, 2019

stale bot removed the stale label Jun 26, 2019

nishantmonu51 force-pushed the jmx-metrics branch from 1c234e0 to be2b9a0 Compare June 26, 2019 19:34

himanshug reviewed Sep 4, 2019

View reviewed changes

rohit.k and others added 12 commits September 24, 2019 18:26

Implementing dropwizard emitter for druid

0817de2

making metric manager and alert emitters as optional

Refactor and make things work

c761731

more improvements improve docs refactrings

Fix teamcity inspections

302a7fe

review comments

6f80eec

more review comments

c6cdf70

add limit to max number of gauges

f5c81fc

update pom version

2ec065a

fix pom

0bfeb0a

review comments

93037bf

review comment

9cb2c89

review comments

2f7a8cd

fix broken doc link

8775f16

review comments review comments

nishantmonu51 force-pushed the jmx-metrics branch from db901a6 to 8775f16 Compare September 24, 2019 12:57

review comments

7a66078

himanshug approved these changes Sep 25, 2019

View reviewed changes

nishantmonu51 added 3 commits September 26, 2019 13:44

fix checkstyle

170bf83

more spell check fixes

7211d70

fix travis failures

22ef36d

himanshug merged commit 8537fbe into apache:master Oct 1, 2019

jon-wei added this to the 0.17.0 milestone Dec 17, 2019

jon-wei mentioned this pull request Dec 28, 2019

0.17.0 release notes #9066

Closed

	druid.emitter.dropwizard.reporter={"type":"jmx"}
	druid.emitter.dropwizard.reporters=[{"type":"jmx"}]

	private final Map<String, Number> gagues;
	private final Map<String, Number> gauges;


		```

		druid.emitter.dropwizard.reporter={"type":"console","emitIntervalInSecs":30}"}

Implementing dropwizard emitter for druid #7363

Implementing dropwizard emitter for druid #7363

Conversation

nishantmonu51 commented Mar 27, 2019

nishantmonu51 commented Apr 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-slim commented Apr 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jun 25, 2019

stale bot commented Jun 26, 2019

stale bot commented Jun 26, 2019

nishantmonu51 commented Sep 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment