Skip to content
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

Deprecating core.instrument.binder and move classes to binder package #3043

Merged
merged 13 commits into from
Feb 28, 2022

Conversation

jonatan-ivanov
Copy link
Member

@jonatan-ivanov jonatan-ivanov commented Feb 28, 2022

Introduces a new module micrometer-binder with all the instrumentation maintained by the Micrometer team. Deprecates the existing instrumentation in micrometer-core. This is a change in preparation for 2.0 where we plan to remove the classes from micrometer-core. This allows consumers of the Micrometer maintained instrumentation in micrometer-core to migrate to the classes in micrometer-binder for a smooth transition to Micrometer 2.0 support.

@jonatan-ivanov jonatan-ivanov added type: task A general task module: micrometer-core An issue that is related to our core module instrumentation An issue that is related to instrumenting a component labels Feb 28, 2022
@jonatan-ivanov jonatan-ivanov added this to the 1.9.0-M4 milestone Feb 28, 2022
@jonatan-ivanov jonatan-ivanov merged commit 598a6b4 into main Feb 28, 2022
@jonatan-ivanov jonatan-ivanov deleted the binders_out branch February 28, 2022 17:32

@NonNullApi
@NonNullFields
class MetricsFilter extends AbstractFilter {
Copy link
Contributor

Choose a reason for hiding this comment

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

ClassCanBeStatic: Inner class is non-static but does not reference enclosing class (details)
(at-me in a reply with help or ignore)

YOUNG,
UNKNOWN;

private static final Map<String, GcGenerationAge> knownCollectors = new HashMap<String, GcGenerationAge>() {{
Copy link
Contributor

Choose a reason for hiding this comment

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

DoubleBraceInitialization: Prefer collection factory methods or builders to the double-brace initialization pattern. (details)
(at-me in a reply with help or ignore)

try {
mBeanServer.removeNotificationListener(
MBeanServerDelegate.DELEGATE_NAME, notificationListener);
} catch (InstanceNotFoundException | ListenerNotFoundException ignore) {
Copy link
Contributor

Choose a reason for hiding this comment

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

EmptyCatch: Caught exceptions should not be ignored (details)
(at-me in a reply with help or ignore)

if (resultSet.next()) {
return resultSet.getLong(1);
}
} catch (SQLException ignored) {
Copy link
Contributor

Choose a reason for hiding this comment

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

EmptyCatch: Caught exceptions should not be ignored (details)
(at-me in a reply with help or ignore)

notificationListenerCleanUpRunnables.add(() -> {
try {
notificationEmitter.removeNotificationListener(gcNotificationListener);
} catch (ListenerNotFoundException ignore) {
Copy link
Contributor

Choose a reason for hiding this comment

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

EmptyCatch: Caught exceptions should not be ignored (details)
(at-me in a reply with help or ignore)

notificationListenerCleanUpRunnables.add(() -> {
try {
notificationEmitter.removeNotificationListener(notificationListener);
} catch (ListenerNotFoundException ignore) {
Copy link
Contributor

Choose a reason for hiding this comment

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

EmptyCatch: Caught exceptions should not be ignored (details)
(at-me in a reply with help or ignore)

for (String className : classNames) {
try {
return Class.forName(className);
} catch (ClassNotFoundException ignore) {
Copy link
Contributor

Choose a reason for hiding this comment

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

EmptyCatch: Caught exceptions should not be ignored (details)
(at-me in a reply with help or ignore)

for (String className : classNames) {
try {
return Class.forName(className);
} catch (ClassNotFoundException ignore) {
Copy link
Contributor

Choose a reason for hiding this comment

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

EmptyCatch: Caught exceptions should not be ignored (details)
(at-me in a reply with help or ignore)

if (!isNotFoundException(event)) {
break;
}
case REQUEST_MATCHED:
Copy link
Contributor

Choose a reason for hiding this comment

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

FallThrough: Execution may fall through from the previous case; add a // fall through comment before this line if it was deliberate (details)
(at-me in a reply with help or ignore)

commonTags = getCommonTags(registry);
prepareToBindMetrics(registry);
checkAndBindMetrics(registry);
scheduler.scheduleAtFixedRate(() -> checkAndBindMetrics(registry), getRefreshIntervalInMillis(), getRefreshIntervalInMillis(), TimeUnit.MILLISECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

FutureReturnValueIgnored: Return value of methods returning Future must be checked. Ignoring returned Futures suppresses exceptions thrown from the code that completes the Future. (details)
(at-me in a reply with help or ignore)

*/
UNKNOWN;

private final Tag tag;
Copy link
Contributor

Choose a reason for hiding this comment

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

ImmutableEnumChecker: enums should be immutable: 'Outcome' has field 'tag' of type 'io.micrometer.core.instrument.Tag', the declaration of type 'io.micrometer.core.instrument.Tag' is not annotated with @com.google.errorprone.annotations.Immutable (details)
(at-me in a reply with help or ignore)

import io.micrometer.core.lang.NonNullFields;

/**
* @author Clint Checketts
Copy link
Contributor

Choose a reason for hiding this comment

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

MissingSummary: A summary line is required on public/protected Javadocs. (details)
(at-me in a reply with help or ignore)

import io.micrometer.core.util.internal.logging.InternalLoggerFactory;

/**
* @author Clint Checketts
Copy link
Contributor

Choose a reason for hiding this comment

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

MissingSummary: A summary line is required on public/protected Javadocs. (details)
(at-me in a reply with help or ignore)

import static java.util.Collections.emptyList;

/**
* @author Jon Schneider
Copy link
Contributor

Choose a reason for hiding this comment

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

MissingSummary: A summary line is required on public/protected Javadocs. (details)
(at-me in a reply with help or ignore)

}

/**
* @return A cache binder bound to a cache named "mycache".
Copy link
Contributor

Choose a reason for hiding this comment

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

MissingSummary: A summary fragment is required; consider using the value of the @return block as a summary fragment instead. (details)
(at-me in a reply with help or ignore)

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

/**
* @author Jon Schneider
Copy link
Contributor

Choose a reason for hiding this comment

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

MissingSummary: A summary line is required on public/protected Javadocs. (details)
(at-me in a reply with help or ignore)

final Function<Code, Timer> cacheResolver = code -> cache.computeIfAbsent(code, creator);
// Eager initialize
for (final Code code : this.eagerInitializedCodes) {
cacheResolver.apply(code);
Copy link
Contributor

Choose a reason for hiding this comment

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

ReturnValueIgnored: Return value of 'apply' must be used (details)
(at-me in a reply with help or ignore)

}
try {
// ensure the Bean we have is actually an instance of the interface
osBeanClass.cast(osBean);
Copy link
Contributor

Choose a reason for hiding this comment

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

ReturnValueIgnored: Return value of 'cast' must be used (details)
(at-me in a reply with help or ignore)

}
try {
// ensure the Bean we have is actually an instance of the interface
operatingSystemBeanClass.cast(operatingSystemBean);
Copy link
Contributor

Choose a reason for hiding this comment

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

ReturnValueIgnored: Return value of 'cast' must be used (details)
(at-me in a reply with help or ignore)

private final DSLContext context;
private final MeterRegistry registry;
private final Iterable<Tag> tags;
private final ThreadLocal<Iterable<Tag>> contextTags = new ThreadLocal<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

ThreadLocalUsage: ThreadLocals should be stored in static fields (details)
(at-me in a reply with help or ignore)

private final String namePrefix;
private final Iterable<Tag> tags;
private final Double maxIdleConnectionCount;
private final ThreadLocal<ConnectionPoolConnectionStats> connectionStats = new ThreadLocal<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

ThreadLocalUsage: ThreadLocals should be stored in static fields (details)
(at-me in a reply with help or ignore)

}

@SuppressWarnings("unchecked")
public <O> O timeCoercable(Object o) {
Copy link
Contributor

Choose a reason for hiding this comment

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

TypeParameterUnusedInFormals: Declaring a type parameter that is only used in the return type is a misuse of generics: operations on the type parameter are unchecked, it hides unsafe casts at invocations of the method, and it interacts badly with method overload resolution. (details)
(at-me in a reply with help or ignore)

.description("Incremented for an increase in the size of the (young) heap memory pool after one GC to before the next")
.register(registry);

promotedBytes = (isGenerationalGc) ? Counter.builder("jvm.gc.memory.promoted").tags(tags)
Copy link
Contributor

Choose a reason for hiding this comment

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

UnnecessaryParentheses: These grouping parentheses are unnecessary; it is unlikely the code will be misinterpreted without them (details)
(at-me in a reply with help or ignore)

* @see #preregisterService(ServerServiceDefinition)
*/
public void preregisterService(final BindableService service) {
preregisterService(service.bindService());
Copy link
Contributor

Choose a reason for hiding this comment

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

INTERFACE_NOT_THREAD_SAFE: Unprotected call to method BindableService.bindService() of un-annotated interface io.grpc.BindableService. Consider annotating the interface with @ThreadSafe or adding a lock.
Reporting because a superclass class io.grpc.ServerInterceptor is annotated @ThreadSafe.
(at-me in a reply with help or ignore)

.description("The number of evictions of near cache entries owned by this member")
.register(registry);

Gauge.builder("cache.near.persistences", cache, cache -> getDouble(cache.getLocalMapStats(), (stats) -> stats.getNearCacheStats().getPersistenceCount()))
Copy link
Contributor

Choose a reason for hiding this comment

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

NULL_DEREFERENCE: object returned by stats.getNearCacheStats() could be null and is dereferenced at line 185.
(at-me in a reply with help or ignore)

private void nearCacheMetrics(MeterRegistry registry) {
LocalMapStats localMapStats = cache.getLocalMapStats();
if (localMapStats != null && localMapStats.getNearCacheStats() != null) {
Gauge.builder("cache.near.requests", cache, cache -> getDouble(cache.getLocalMapStats(), (stats) -> stats.getNearCacheStats().getHits()))
Copy link
Contributor

Choose a reason for hiding this comment

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

NULL_DEREFERENCE: object returned by stats.getNearCacheStats() could be null and is dereferenced at line 170.
(at-me in a reply with help or ignore)

.description("The number of hits (reads) of near cache entries owned by this member")
.register(registry);

Gauge.builder("cache.near.requests", cache, cache -> getDouble(cache.getLocalMapStats(), (stats) -> stats.getNearCacheStats().getMisses()))
Copy link
Contributor

Choose a reason for hiding this comment

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

NULL_DEREFERENCE: object returned by stats.getNearCacheStats() could be null and is dereferenced at line 175.
(at-me in a reply with help or ignore)

.description("The number of hits (reads) of near cache entries owned by this member")
.register(registry);

Gauge.builder("cache.near.evictions", cache, cache -> getDouble(cache.getLocalMapStats(), (stats) -> stats.getNearCacheStats().getEvictions()))
Copy link
Contributor

Choose a reason for hiding this comment

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

NULL_DEREFERENCE: object returned by stats.getNearCacheStats() could be null and is dereferenced at line 180.
(at-me in a reply with help or ignore)

this.scheduler.shutdownNow();

for (Meter.Id id : registeredMeterIds) {
registry.remove(id);
Copy link
Contributor

Choose a reason for hiding this comment

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

NULL_DEREFERENCE: object KafkaMetrics.registry last accessed on line 306 could be null and is dereferenced at line 306.
(at-me in a reply with help or ignore)

tags.add(Tag.of(KAFKA_VERSION_TAG_NAME, kafkaVersion));
extraTags.forEach(tags::add);
if (includeCommonTags) {
commonTags.forEach(tags::add);
Copy link
Contributor

Choose a reason for hiding this comment

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

NULL_DEREFERENCE: object KafkaMetrics.commonTags could be null and is dereferenced at line 283.
(at-me in a reply with help or ignore)

if (cs.get() == null) {
cs.set(new ConnectionPoolConnectionStats());
}
return cs.get().getActiveCount();
Copy link
Contributor

Choose a reason for hiding this comment

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

NULL_DEREFERENCE: object returned by cs.get() could be null and is dereferenced at line 124.
(at-me in a reply with help or ignore)

shakuzen added a commit that referenced this pull request Mar 3, 2022
shakuzen pushed a commit that referenced this pull request Mar 3, 2022
…#3043)

* Moving binders out

we have copied over the binders from core to a separate module called micrometer-binders
binders in micrometer-core will be deprecated
the migration path will be for the users to use the micrometer-binders jar together with micrometer-core if necessary

* Added deprecations

* Move binders out from the TCK too

* Use io.micrometer.binder.jvm in JvmServiceLevelObjectives

* Use io.micrometer.binder in the registries (Health and StatsD)

* Use io.micrometer.binder in core

* Use io.micrometer.binder in samples

* Add deprecated note to javadoc with a suggestion to resolve it

* Deleting javadoc comments from binder package-info
I guess this was a c/p error.

* Delete DiskSpaceMetrics that we deprecated earlier

* Delete JettyStatisticsMetrics that we deprecated earlier

* Delete KafkaConsumerMetrics that we deprecated earlier

* Delete hibernate metrics that we deprecated earlier

Co-authored-by: Marcin Grzejszczak <mgrzejszczak@vmware.com>
@shakuzen shakuzen added enhancement A general enhancement and removed type: task A general task labels Mar 17, 2022
shakuzen added a commit that referenced this pull request Apr 12, 2022
marcingrzejszczak added a commit that referenced this pull request Apr 12, 2022
marcingrzejszczak added a commit that referenced this pull request Apr 12, 2022
* Added compatibility plugin

* Revert "Remove deprecated MissingRequiredConfigurationException and its usages (#2987)"

This reverts commit fde4f3d.

* Revert "Remove StatsdConfig.queueSize() (#568)"

This reverts commit 0c919bb.

* Revert "Remove deprecated methods in GangliaConfig"

This reverts commit a8ebe88.

* Revert "Remove deprecated ElasticConfig#documentType"

This reverts commit dfab16a.

* Revert "Remove deprecated constructor and dropwizard dependency"

This reverts commit acb6d7f.

* Revert "Remove deprecated method newLongTaskTimer(Id) on MeterRegistry"

This reverts commit 852991d.

* Revert "Remove Hibernate optional dependency"

This reverts commit 575b85e.

* Revert "Remove deprecated KafkaConsumerMetrics"

This reverts commit 207bb38.

* Revert "Remove deprecated JettyStatisticsMetrics"

This reverts commit 0902d4a.

* Revert "Remove deprecated Hibernate metrics"

This reverts commit 31ad59f.

* Revert "Remove deprecated DiskSpaceMetrics"

This reverts commit 46c8d37.

* Revert "Delete deprecated binders"

This reverts commit f8a508d.

* Revert "Deprecating core.instrument.binder and move classes to binder package (#3043)"

This reverts commit 531cb87.

* Revert "Remove things added back in merge by mistake."

This reverts commit 4cc4c20.

* Ensures binary compatibility

* 1.10.x KeyValue & KeyValues (#3122)

* Trying to fix the Tag & Tags confusion

- we rename Tag & Tags into KeyValue and KeyValues - that way there will be no confusions between the current Tag(s) and the ones from the common module
- TagKey will have the "of" method removed - that way we will not be returning Tags from commons that can be then used by micrometer's core code
- Changed since from 2.0.0 to 1.10.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement instrumentation An issue that is related to instrumenting a component module: micrometer-core An issue that is related to our core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants