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

Merge micrometer-binder changes and revert micrometer-api changes #3046

Merged
merged 22 commits into from Mar 3, 2022

Conversation

jonatan-ivanov
Copy link
Member

@jonatan-ivanov jonatan-ivanov commented Mar 1, 2022

Reverts the micrometer-api module in favor of the micrometer-binder module, merging changes from that.

See #2980 and #3043

jeremyross and others added 8 commits February 16, 2022 17:09
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 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 {
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)

ContainerRequest containerRequest = event.getContainerRequest();
Set<Timed> timedAnnotations;

switch (event.getType()) {
Copy link
Contributor

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
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.lang.NonNullFields;

/**
* @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)

}

/**
* @param tags Additional tags which should be exposed with every value.
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)

}

/**
* @param waitForContinue Overrides the wait for continue time for this
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)

}

/**
* @return Creates an instance of {@link MicrometerHttpRequestExecutor}
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 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.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 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)

do {
prev = exemplar.get();
next = exemplarSampler.sample(amount, prev);
if (next == null || next == prev) {
Copy link
Contributor

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);
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)

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)

if (cs.get() == null) {
cs.set(new ConnectionPoolConnectionStats());
}
return cs.get().getIdleConnectionCount();
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 136.
(at-me in a reply with help or ignore)

shakuzen and others added 9 commits March 3, 2022 22:12
Without this, tests fail on Apple silicon due to the missing compatible native library.

See gh-3048
The versions we were on used deprecated Gradle API and they logged warnings on Apple silicon. Both of these issues are resolved in these latest versions of the plugins.

See gh-3048
Resolves gh-3049
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.
Copy link
Member Author

@jonatan-ivanov jonatan-ivanov left a comment

Choose a reason for hiding this comment

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

looks good

jonatan-ivanov and others added 4 commits March 4, 2022 02:00
…#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
The versions we were on used deprecated Gradle API and they logged warnings on Apple silicon. Both of these issues are resolved in these latest versions of the plugins.

See gh-3048
Resolves gh-3049
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.
@shakuzen shakuzen marked this pull request as ready for review March 3, 2022 17:28
@shakuzen shakuzen merged commit 6492bee into 2.0.x Mar 3, 2022
@jonatan-ivanov jonatan-ivanov deleted the api-back-to-core branch March 3, 2022 17:43
@shakuzen shakuzen added this to the 2.0.0-M3 milestone Mar 15, 2022
@shakuzen shakuzen added module: micrometer-core An issue that is related to our core module module: micrometer-api labels Mar 15, 2022
@shakuzen shakuzen changed the title Merge micrometer-binder changes with micrometer-api changes Merge micrometer-binder changes and revert micrometer-api changes Mar 15, 2022
@shakuzen shakuzen added enhancement A general enhancement release notes Noteworthy change to call out in the release notes labels Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement module: micrometer-api module: micrometer-core An issue that is related to our core module release notes Noteworthy change to call out in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants