Add basic metrics support #360

felixbarny · 2018-11-29T16:22:23Z

No description provided.

felixbarny · 2018-12-13T15:33:01Z

Adds the following system metrics:

system.cpu.total.norm.pct
system.process.cpu.total.norm.pct
system.memory.total
system.memory.actual.free
system.process.memory.size

And some heap metrics:

jvm.memory.heap.used
jvm.memory.heap.committed
jvm.memory.heap.max
jvm.memory.non_heap.used
jvm.memory.non_heap.committed
jvm.memory.non_heap.max

felixbarny · 2018-12-13T15:38:49Z

@kuisathaverat Do you know why pipeline Jenkins does not like this PR? I probably confused it with my wild force pushes 😬

kuisathaverat · 2018-12-13T17:32:59Z

it looks like JENKINS-44598, probably is related to the force push, I never see it before.

eyalkoren

Great!
Most importantly - used getNonHeapMemoryUsage twice.
Other than that, I am really missing some documentation about central classes role and data structures.
And some cosmetic suggestions.

apm-agent-core/src/main/java/co/elastic/apm/agent/metrics/MetricRegistry.java

eyalkoren · 2018-12-16T16:56:46Z

apm-agent-core/src/main/java/co/elastic/apm/agent/metrics/MetricSet.java

+    }
+
+    public void serialize(long epochMicros, StringBuilder replaceBuilder, JsonWriter jw) {
+        jw.writeByte(JsonWriter.OBJECT_START);


Same - serialization better be colocated

apm-agent-core/src/main/java/co/elastic/apm/agent/metrics/MetricSet.java

eyalkoren · 2018-12-16T17:29:52Z

apm-agent-core/src/main/java/co/elastic/apm/agent/metrics/builtin/JvmMemoryMetrics.java

+import java.lang.management.MemoryUsage;
+import java.util.Collections;
+
+public class JvmMemoryMetrics implements LifecycleListener {


Not sure why this is a LifecycleListener if the MBeans are just registered on startup. Not very important but if there is no reason- consider changing.

We already have the concept of LifecycleListeners which provide a callback on application start and on shutdown. I did not create an SPI for metrics as I could just use the already existing one. If you think it would make things more clear, I can add a special SPI just for metrics.

eyalkoren · 2018-12-16T17:42:27Z

apm-agent-core/src/main/java/co/elastic/apm/agent/metrics/builtin/SystemMetrics.java

+ * This implementation is based on io.micrometer.core.instrument.binder.system.ProcessorMetrics,
+ * under Apache License 2.0
+ */
+public class SystemMetrics implements LifecycleListener {


Same- consider not making a listener. I assume it is enough that the metrics collected thread is shut down when required.

apm-agent-core/src/main/java/co/elastic/apm/agent/metrics/builtin/JvmMemoryMetrics.java

felixbarny · 2018-12-17T13:28:31Z

I have also added GC metrics:

jvm.gc.count [tags: name]
jvm.gc.time [tags: name]
jvm.gc.alloc

felixbarny · 2018-12-18T09:10:37Z

I have manually checked that system.cpu.total.norm.pct returns similar values to the ones emitted by our Go agent.

eyalkoren

Some minor comments-

Suggesting small serialization-dependency erasure
Can we remove PayloadSender?
It just occurred to me that the entire metrics harvesting is going to be done on the disruptor publishing thread. Do you think this may be problematic and be better done offline, so not to hold down other event publishing?

apm-agent-core/src/main/java/co/elastic/apm/agent/metrics/MetricRegistry.java

eyalkoren · 2018-12-19T11:17:44Z

apm-agent-core/src/main/java/co/elastic/apm/agent/metrics/MetricRegistry.java

+    }
+
+    @Override
+    public String toString() {


Moving the serialization code to the serialize package is great, so I think having this JsonWriter residual here is a shame. Sorry for nagging, I have future refactors in mind...
What do you say adding a static asString(MetricsRegistry) to the serializer?

do you want to create a separate module for serialization?

Not at the moment :)
I just think it makes sense to avoid dependency on serialization lib here.

I'm all about keeping package dependencies uni-directional!

eyalkoren · 2018-12-19T11:41:31Z

apm-agent-core/src/main/java/co/elastic/apm/agent/metrics/builtin/JvmGcMetrics.java

+            registry.addUnlessNegative("jvm.gc.count", tags, new DoubleSupplier() {
+                @Override
+                public double get() {
+                    return garbageCollectorMXBean.getCollectionCount();


What would be more interesting - collection count or collection count since last measurement? For graphs, an ever increasing one is not as informative as a mostly-stagnant one where spikes are easier to spot. This is something that can be calculated in the server or Kibana as well, but since it is not a global metric, I don't think it would be.

It's quite common to report counters and use a derivative aggregation to graph the deltas.

eyalkoren · 2018-12-19T11:41:49Z

apm-agent-core/src/main/java/co/elastic/apm/agent/metrics/builtin/JvmGcMetrics.java

+            registry.addUnlessNegative("jvm.gc.time", tags, new DoubleSupplier() {
+                @Override
+                public double get() {
+                    return garbageCollectorMXBean.getCollectionTime();


felixbarny · 2018-12-19T13:21:10Z

It just occurred to me that the entire metrics harvesting is going to be done on the disruptor publishing thread. Do you think this may be problematic and be better done offline, so not to hold down other event publishing?

I'd think that getting and serializing the metrics is not something that would hold down the reporter thread. I think the bottleneck there is the network anyway.

Do you think this may be problematic and be better done offline

Do you have an idea how to do this without serializing the metrics to a buffer which becomes garbage afterwards and without establishing another connection to the APM Server?

eyalkoren · 2018-12-19T13:42:33Z

I am not talking about serialization, I am just talking about metrics collection from all MBeans. Anyway, as long as we don't have evidence that this is a problem - it is not.

felixbarny added the [zube]: In Progress label Nov 29, 2018

felixbarny mentioned this pull request Nov 29, 2018

Support Elastic APM micrometer-metrics/micrometer#793

Closed

felixbarny self-assigned this Nov 30, 2018

felixbarny force-pushed the application-metrics branch 2 times, most recently from 93d1e42 to 156db92 Compare December 13, 2018 15:15

Add basic metrics

49a0efd

felixbarny force-pushed the application-metrics branch from 84a8d05 to 49a0efd Compare December 13, 2018 15:29

felixbarny changed the title ~~WIP: metrics~~ Add basic metrics support Dec 13, 2018

Make animal sniffer happy

85c50cb

felixbarny requested a review from eyalkoren December 13, 2018 15:46

felixbarny added [zube]: In Review and removed [zube]: In Progress labels Dec 13, 2018

eyalkoren reviewed Dec 17, 2018

View reviewed changes

apm-agent-core/src/main/java/co/elastic/apm/agent/metrics/builtin/JvmMemoryMetrics.java Outdated Show resolved Hide resolved

felixbarny added 3 commits December 17, 2018 12:29

Add metrics for GC and allocations

4f180ac

Add javadoc

35f1006

Move serialization code out of MetricRegistry/MetricSet

d012c59

felixbarny added 3 commits December 17, 2018 16:51

Add documentation for metrics

472c2ec

Make animal sniffer happy

06ae62b

Add to metrics_interval config from metrics doc

4fd650b

felixbarny added 2 commits December 18, 2018 14:30

Actualy report updated memory metrics

af94b5a

Merge remote-tracking branch 'origin/master' into application-metrics

fbc389c

eyalkoren reviewed Dec 19, 2018

View reviewed changes

Review comments

20ffadb

eyalkoren approved these changes Dec 19, 2018

View reviewed changes

felixbarny merged commit 8d31b40 into elastic:master Dec 19, 2018

felixbarny deleted the application-metrics branch December 19, 2018 14:09

zube bot added [zube]: Done and removed [zube]: In Review labels Dec 19, 2018

felixbarny mentioned this pull request Dec 19, 2018

Add support for Metrics / Stats API #112

Closed

alvarolobato removed the [zube]: Done label Jan 14, 2019

gregkalapos mentioned this pull request Mar 21, 2019

Implement metrics elastic/apm-agent-dotnet#154

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic metrics support #360

Add basic metrics support #360

felixbarny commented Nov 29, 2018

felixbarny commented Dec 13, 2018 •

edited

Loading

felixbarny commented Dec 13, 2018

kuisathaverat commented Dec 13, 2018

eyalkoren left a comment •

edited

Loading

eyalkoren Dec 16, 2018

eyalkoren Dec 16, 2018

felixbarny Dec 17, 2018

eyalkoren Dec 16, 2018

felixbarny commented Dec 17, 2018

felixbarny commented Dec 18, 2018

eyalkoren left a comment

eyalkoren Dec 19, 2018

felixbarny Dec 19, 2018

eyalkoren Dec 19, 2018

felixbarny Dec 19, 2018

eyalkoren Dec 19, 2018 •

edited

Loading

felixbarny Dec 19, 2018

eyalkoren Dec 19, 2018

felixbarny commented Dec 19, 2018

eyalkoren commented Dec 19, 2018

Add basic metrics support #360

Add basic metrics support #360

Conversation

felixbarny commented Nov 29, 2018

felixbarny commented Dec 13, 2018 • edited Loading

felixbarny commented Dec 13, 2018

kuisathaverat commented Dec 13, 2018

eyalkoren left a comment • edited Loading

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

felixbarny commented Dec 17, 2018

felixbarny commented Dec 18, 2018

eyalkoren left a 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

eyalkoren Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixbarny commented Dec 19, 2018

eyalkoren commented Dec 19, 2018

felixbarny commented Dec 13, 2018 •

edited

Loading

eyalkoren left a comment •

edited

Loading

eyalkoren Dec 19, 2018 •

edited

Loading