-
Notifications
You must be signed in to change notification settings - Fork 981
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
Merge micrometer-binder changes and revert micrometer-api changes #3046
Conversation
Allows data to be written to indexes or data streams. Resolves gh-2996 Co-authored-by: Tommy Ludwig <8924140+shakuzen@users.noreply.github.com>
This reverts commit 6dc8222.
This commit also updates the existing integration tests to use Elasticsearch 7.17.0.
…#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>
Reverts the micrometer-api module in favor of the micrometer-binder module, merging changes from that.
|
||
@NonNullApi | ||
@NonNullFields | ||
class MetricsFilter extends AbstractFilter { |
There was a problem hiding this comment.
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>() {{ |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
)
ContainerRequest containerRequest = event.getContainerRequest(); | ||
Set<Timed> timedAnnotations; | ||
|
||
switch (event.getType()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MissingCasesInEnumSwitch: Non-exhaustive switch; either add a default or handle the remaining cases: START, MATCHING_START, LOCATOR_MATCHED, and 8 others (details)
(at-me in a reply with help
or ignore
)
import io.micrometer.core.lang.Nullable; | ||
|
||
/** | ||
* @author Jon Schneider |
There was a problem hiding this comment.
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.lang.NonNullFields; | ||
|
||
/** | ||
* @author Jon Schneider |
There was a problem hiding this comment.
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
)
} | ||
|
||
/** | ||
* @param tags Additional tags which should be exposed with every value. |
There was a problem hiding this comment.
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
)
} | ||
|
||
/** | ||
* @param waitForContinue Overrides the wait for continue time for this |
There was a problem hiding this comment.
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
)
} | ||
|
||
/** | ||
* @return Creates an instance of {@link MicrometerHttpRequestExecutor} |
There was a problem hiding this comment.
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 io.micrometer.core.lang.NonNullFields; | ||
|
||
/** | ||
* @author Clint Checketts |
There was a problem hiding this comment.
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.lang.NonNullFields; | ||
|
||
/** | ||
* @author Clint Checketts |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
)
do { | ||
prev = exemplar.get(); | ||
next = exemplarSampler.sample(amount, prev); | ||
if (next == null || next == prev) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ReferenceEquality: Comparison using reference equality instead of value equality (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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
)
tags.add(Tag.of(KAFKA_VERSION_TAG_NAME, kafkaVersion)); | ||
extraTags.forEach(tags::add); | ||
if (includeCommonTags) { | ||
commonTags.forEach(tags::add); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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
)
if (cs.get() == null) { | ||
cs.set(new ConnectionPoolConnectionStats()); | ||
} | ||
return cs.get().getIdleConnectionCount(); |
There was a problem hiding this comment.
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 136.
(at-me in a reply with help
or ignore
)
...er-core/src/main/java/io/micrometer/core/instrument/binder/jetty/JettyStatisticsMetrics.java
Outdated
Show resolved
Hide resolved
...er-core/src/main/java/io/micrometer/core/instrument/transport/http/tags/HttpRequestTags.java
Outdated
Show resolved
Hide resolved
Without this, tests fail on Apple silicon due to the missing compatible native library. See gh-3048
These were previously removed in the 2.0.x branch since they were deprecated in 1.x for a while already.
Core should contain the classes needed to write instrumentation, and binders should have instrumentation that we maintain. Therefore, the `CacheMeterBinder` and HTTP-related classes should remain in core. Related to fixing the cache stuff, this also moves the cache TCK implementations to micrometer-binders where the implementations of the cache binders are.
This reverts commit 6dc8222.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
…#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>
Without this, tests fail on Apple silicon due to the missing compatible native library. See gh-3048
Core should contain the classes needed to write instrumentation, and binders should have instrumentation that we maintain. Therefore, the `CacheMeterBinder` and HTTP-related classes should remain in core. Related to fixing the cache stuff, this also moves the cache TCK implementations to micrometer-binders where the implementations of the cache binders are.
dc28d83
to
dadb6bb
Compare
Reverts the micrometer-api module in favor of the micrometer-binder module, merging changes from that.
See #2980 and #3043