Skip to content

Graphite emitter#1978

Merged
b-slim merged 1 commit intoapache:masterfrom
b-slim:graphite_emitter
Jan 21, 2016
Merged

Graphite emitter#1978
b-slim merged 1 commit intoapache:masterfrom
b-slim:graphite_emitter

Conversation

@b-slim
Copy link
Copy Markdown
Contributor

@b-slim b-slim commented Nov 17, 2015

Adding a new emitter that sends some carefully selected metrics to a graphite server.

@b-slim
Copy link
Copy Markdown
Contributor Author

b-slim commented Nov 17, 2015

@rohitkochar please checkout this and let me know what you think

Comment thread extensions/graphite-emitter/README.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might need a blank line before the table starts for it to render correctly

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would read better if you said "All the configuration parameters ...". That would solve the subject-verb number disagreement here.

It would be good to have some punctuation at the end, maybe a colon.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@rohitkochar
Copy link
Copy Markdown
Contributor

Thanks @b-slim .
I will definitely review this and let you know my feedback.

Comment thread extensions/graphite-emitter/README.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of module README, please put this in a the doc file in the docs/development folder and link in TOC as an experimental feature

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@himanshug
Copy link
Copy Markdown
Contributor

This may be unrelated to this PR but,
I asked @b-slim to take a look at https://github.com/dropwizard/metrics which is a metrics emitting library that supports reporting to multiple different projects e.g. graphite, ganglia, file, jmx etc etc. if this emitter uses same then you would automatically support a whole bunch of monitoring software and not just graphite.
That said, We do not have any personal experience with that library and not sure how good the community around that project is. If someone has used it in the past then please speak up.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you can probably just do

catch (ExecutionException | TimeoutException e) {
        log.error(e, e.getMessage());
    }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting issue

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@sixtus
Copy link
Copy Markdown

sixtus commented Nov 20, 2015

@himanshug and @b-slim - we are using https://github.com/dropwizard/metrics in production for ages now. Just works. I guess it's finally time to write our own emitter (dropwizard -> kafka), I was holding it off for druid 0.8 changes.

@himanshug
Copy link
Copy Markdown
Contributor

@sixtus created #1996 to track dropwizard/metrics based emitter . if you have something please contribute that as an extension.

Comment thread extensions/graphite-emitter/README.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the comma here should be a semicolon, and no space before it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@b-slim
Copy link
Copy Markdown
Contributor Author

b-slim commented Jan 20, 2016

@himanshug can you please review this ?

Comment thread docs/content/configuration/index.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

md or html?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fjy though the all the MD files are converted to html, ? i am i wrong ?

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Jan 21, 2016

👍 after my comment around the README fix is addressed

Comment thread extensions/graphite-emitter/README.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we just link to the json file containing the whitelist or else the json below has to be kept in sync

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@himanshug
Copy link
Copy Markdown
Contributor

@bslim 👍 besides the minor comments, feel free to merge after resolving the comments

@b-slim
Copy link
Copy Markdown
Contributor Author

b-slim commented Jan 21, 2016

@rohitkochar are you aware of the current changes on how to supply the whiteList map

@b-slim
Copy link
Copy Markdown
Contributor Author

b-slim commented Jan 21, 2016

@spektom we changed the way we supply the map in favor of file path.

@b-slim b-slim closed this Jan 21, 2016
@b-slim b-slim reopened this Jan 21, 2016
b-slim added a commit that referenced this pull request Jan 21, 2016
@b-slim b-slim merged commit a752bc0 into apache:master Jan 21, 2016
@fjy fjy mentioned this pull request Feb 5, 2016
@l15k4
Copy link
Copy Markdown

l15k4 commented May 3, 2016

I added the graphite extension but now even though it is present :

root@druid:/# ls -la /root/imply-1.2.0/dist/druid/extensions/graphite-emitter/graphite-emitter-0.9.0.jar
-rw-r--r-- 1 root root 22022 May  3 11:44 /root/imply-1.2.0/dist/druid/extensions/graphite-emitter/graphite-emitter-0.9.0.jar

I get this error :

Caused by: java.io.FileNotFoundException: file:/root/imply-1.2.0/dist/druid/extensions/graphite-emitter/graphite-emitter-0.9.0.jar!/defaultWhiteListMap.json (No such file or directory)
        at java.io.FileInputStream.open0(Native Method)
        at java.io.FileInputStream.open(FileInputStream.java:195)
        at java.io.FileInputStream.<init>(FileInputStream.java:138)
        at com.google.common.io.Files$FileByteSource.openStream(Files.java:126)
        at com.google.common.io.Files$FileByteSource.openStream(Files.java:116)
        at com.google.common.io.ByteSource$AsCharSource.openStream(ByteSource.java:434)
        at com.google.common.io.CharSource.read(CharSource.java:161)
        at io.druid.emitter.graphite.WhiteListBasedConverter.readMap(WhiteListBasedConverter.java:256)
        at io.druid.emitter.graphite.WhiteListBasedConverter.<init>(WhiteListBasedConverter.java:86)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
        at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
        at com.fasterxml.jackson.databind.introspect.AnnotatedConstructor.call(AnnotatedConstructor.java:125)
        at com.fasterxml.jackson.databind.deser.std.StdValueInstantiator.createFromObjectWith(StdValueInstantiator.java:230)

The default defaultWhiteListMap.json is actually present in the jar file :

# jar -tf graphite-emitter-0.9.0.jar 
META-INF/
META-INF/MANIFEST.MF
io/
io/druid/
io/druid/emitter/
io/druid/emitter/graphite/
META-INF/services/
defaultWhiteListMap.json
io/druid/emitter/graphite/DruidToGraphiteEventConverter.class
io/druid/emitter/graphite/GraphiteEmitter$1.class
io/druid/emitter/graphite/GraphiteEmitter$ConsumerRunnable.class
io/druid/emitter/graphite/GraphiteEmitter.class
io/druid/emitter/graphite/GraphiteEmitterConfig.class
io/druid/emitter/graphite/GraphiteEmitterModule$1.class
io/druid/emitter/graphite/GraphiteEmitterModule.class
io/druid/emitter/graphite/GraphiteEvent.class
io/druid/emitter/graphite/SendAllGraphiteEventConverter.class
io/druid/emitter/graphite/WhiteListBasedConverter$1.class
io/druid/emitter/graphite/WhiteListBasedConverter.class
META-INF/services/io.druid.initialization.DruidModule
META-INF/maven/
META-INF/maven/io.druid.extensions.contrib/
META-INF/maven/io.druid.extensions.contrib/graphite-emitter/
META-INF/maven/io.druid.extensions.contrib/graphite-emitter/pom.xml
META-INF/maven/io.druid.extensions.contrib/graphite-emitter/pom.properties

It is just cannot be loaded. Btw the graphite-extension is part of the docker image and druid.extensions.loadList

@l15k4
Copy link
Copy Markdown

l15k4 commented May 3, 2016

I think it's a bug, because :

new File(actualPath)

cannot read a path gotten from :

getClass().getClassLoader().getResource("defaultWhiteListMap.json").getFile();

https://github.com/druid-io/druid/blob/45c413af7e4449153869eee889e1fb30e8817828/extensions-contrib/graphite-emitter/src/main/java/io/druid/emitter/graphite/WhiteListBasedConverter.java#L256

So it basically expect users to always supply custom defaultWhiteListMap.json :

druid.emitter.graphite.eventConverter={"type":"whiteList", "namespacePrefix": "druid", "ignoreHostname":false, "ignoreServiceName":false, "mapPath": "dist/druid/extensions/graphite-emitter/defaultWhiteListMap.json"}

@b-slim
Copy link
Copy Markdown
Contributor Author

b-slim commented May 3, 2016

@l15k4 are you sure your user used to run druid is able to read the file ?

@l15k4
Copy link
Copy Markdown

l15k4 commented May 3, 2016

@b-slim yeah it's a docker container, everything runs as root. Imho it is clear that :

new File("file:/root/imply-1.2.0/dist/druid/extensions/graphite-emitter/graphite-emitter-0.9.0.jar!/defaultWhiteListMap.json")

Cannot work ... java.io.File cannot read resources from jar files....

@b-slim
Copy link
Copy Markdown
Contributor Author

b-slim commented May 3, 2016

@l15k4 will take a look thanks for you concern !

@l15k4
Copy link
Copy Markdown

l15k4 commented May 3, 2016

@b-slim thanks, it is weird that it works for others though. I'm using the implydata distribution and docker-distribution image.

I had to modify it :

RUN java -classpath "/root/imply-$implyversion/dist/druid/lib/*" io.druid.cli.Main tools pull-deps --defaultVersion $druidversion -c io.druid.extensions.contrib:graphite-emitter \
    && mv extensions/graphite-emitter dist/druid/extensions \
    && rm -rf hadoop-dependencies
COPY defaultWhiteListMap.json dist/druid/extensions/graphite-emitter/

Btw would you mind giving me a hand here? https://groups.google.com/d/msg/druid-user/RnpKpW5vowg/0sTc4syLBAAJ

@b-slim
Copy link
Copy Markdown
Contributor Author

b-slim commented May 3, 2016

@l15k4 please check this fix #2917.

@l15k4
Copy link
Copy Markdown

l15k4 commented May 10, 2016

Hey @b-slim, I've noticed of another problem, major one, when graphite becomes unreachable for some time (i just stopped it for a few minutes) then even though it becomes reachable the GraphiteEmitter cannot recover from that and it indefinitely throws exceptions every minute :

2016-05-10T17:42:55,419 ERROR [GraphiteEmitter-0] io.druid.emitter.graphite.GraphiteEmitter - Broken pipe
java.net.SocketException: Broken pipe
        at java.net.SocketOutputStream.socketWrite0(Native Method) ~[?:1.8.0_92]
        at java.net.SocketOutputStream.socketWrite(SocketOutputStream.java:109) ~[?:1.8.0_92]
        at java.net.SocketOutputStream.write(SocketOutputStream.java:141) ~[?:1.8.0_92]
        at com.codahale.metrics.graphite.PickledGraphite.writeMetrics(PickledGraphite.java:262) ~[metrics-graphite-3.1.2.jar:3.1.2]
        at com.codahale.metrics.graphite.PickledGraphite.flush(PickledGraphite.java:221) ~[metrics-graphite-3.1.2.jar:3.1.2]
        at io.druid.emitter.graphite.GraphiteEmitter$ConsumerRunnable.run(GraphiteEmitter.java:172) [graphite-emitter-0.9.0.jar:0.9.0]
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) [?:1.8.0_92]
        at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308) [?:1.8.0_92]
        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180) [?:1.8.0_92]
        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294) [?:1.8.0_92]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) [?:1.8.0_92]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) [?:1.8.0_92]
        at java.lang.Thread.run(Thread.java:745) [?:1.8.0_92]

The PickledGraphite#socker breaks and it is trying to use it all over again. The connection should be closed and new one created I guess.

@b-slim
Copy link
Copy Markdown
Contributor Author

b-slim commented May 10, 2016

@l15k4 can you test this patch #2952

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants