Skip to content

Add Compilation JVM metric binder #1589

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 16, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/**
* Copyright 2017 Pivotal Software, Inc.
* <p>
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* <p>
* https://www.apache.org/licenses/LICENSE-2.0
* <p>
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.micrometer.core.instrument.binder.jvm;

import io.micrometer.core.instrument.FunctionCounter;
import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.Tag;
import io.micrometer.core.instrument.Tags;
import io.micrometer.core.instrument.binder.MeterBinder;
import io.micrometer.core.lang.NonNullApi;
import io.micrometer.core.lang.NonNullFields;

import java.lang.management.CompilationMXBean;
import java.lang.management.ManagementFactory;

import static java.util.Collections.emptyList;

@NonNullApi
@NonNullFields
public class CompilationMetrics implements MeterBinder {
Copy link
Contributor

Choose a reason for hiding this comment

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

@shakuzen @jkschneider Should this class be named JvmCompilationMetrics since these are metrics around the JVM?

Copy link
Contributor Author

@gquintana gquintana Sep 17, 2019

Choose a reason for hiding this comment

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

I don't mind renaming the class but there is already a ClassLoaderMetrics class which should be renamed JvmClassLoaderMetrics in that case.

Copy link
Member

Choose a reason for hiding this comment

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

I agree JvmCompilationMetrics is more clear here. ClassLoaderMetrics is already released public API so we can't change that without breaking all users of it.

private final Iterable<Tag> tags;

public CompilationMetrics() {
this(emptyList());
}

public CompilationMetrics(Iterable<Tag> tags) {
this.tags = tags;
}

@Override
public void bindTo(MeterRegistry registry) {
CompilationMXBean compilationBean = ManagementFactory.getCompilationMXBean();
if (compilationBean.isCompilationTimeMonitoringSupported()) {
FunctionCounter.builder("jvm.compilation.time.total", compilationBean, CompilationMXBean::getTotalCompilationTime)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am unsure about the naming and the baseUnit usage below.
The PrometheusNamingConvention adds the unit and then the "_total" suffix again due to how it works for Counters:
jvm_compilation_time_total_ms_total

Unfortunately we can't make this a Timer due to not being triggered by the JVM so we don't really have any counts on compilation frequency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about jvm.compilation.ms.total ?

Copy link
Contributor

Choose a reason for hiding this comment

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

That will become jvm_compilation_ms_total_ms_total because the PrometheusNamingConvention will first add the base unit and then the _total.

How about just jvm.compilation.time and any NamingConvention specific to a registry implementation will make sure that it generates whatever is best practice for that backend. For Prometheus this would then become jvm_compilation_time_ms_total.

Copy link
Member

Choose a reason for hiding this comment

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

jvm.compilation.time sounds good to me.

.tags(Tags.concat(tags, "compiler", compilationBean.getName()))
.description("The approximate accumulated elapsed time spent in compilation")
.baseUnit("ms")
.register(registry);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
* Copyright 2017 Pivotal Software, Inc.
* <p>
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* <p>
* https://www.apache.org/licenses/LICENSE-2.0
* <p>
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.micrometer.core.instrument.binder.jvm;

import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

class CompilationMetricsTest {
@Test
void compilationTimeMetric() {
MeterRegistry registry = new SimpleMeterRegistry();
new CompilationMetrics().bindTo(registry);

assertThat(registry.get("jvm.compilation.time.total").functionCounter().count()).isGreaterThan(0);
}
}