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

Add dynamic metric support for gauges #15782

Merged
merged 3 commits into from Oct 24, 2019
Merged

Conversation

@blazember
Copy link
Contributor

blazember commented Oct 16, 2019

This change adds dynamic metric support for gauges. Prior to this change
gauges were updated from static metrics only.

Dynamic metrics are supported by introducing MetricValueCatcher that
catches the values of the dynamic metrics during the collection cycle.

The dynamic metrics provided as concrete values by calling
collect(MetricTagger, String, ProbeLevel, ProbeUnit, long) or
collect(MetricTagger, String, ProbeLevel, ProbeUnit, double) on
MetricsCollectionContext are cached by values, so the gauge.read()
calls return the last updated value for the metric.

The dynamic metrics provided through collected objects by calling
MetricsCollectionContext#collect(MetricTagger, Object) are cached by
source object. Once the given metric source object is discovered in a
collection cycle, the object gets cached inside the gauge and every
gauge#read() returns the most current value of the metric. This
caching doesn't prevent the source object to get GCd since the gauges
reference it through WeakReferences.

Besides allowing the metric source object to get GCd, the gauges that
are not "visited" during a metric collection cycle get reset therefore
no stale values are read once the backing dynamic metrics sources
disappear. This is true even if the metric source object is still alive.

EE PR: hazelcast/hazelcast-enterprise#3274

Fixes #15718
Fixes hazelcast/hazelcast-enterprise/issues/3216

@blazember blazember added this to the 4.0 milestone Oct 16, 2019
@blazember blazember self-assigned this Oct 16, 2019
@blazember blazember force-pushed the blazember:4.0/fix/gh-15718 branch from c24fce4 to e1dc17a Oct 16, 2019
Copy link
Contributor

puzpuzpuz left a comment

Looks good to me. Left a couple of minor comments for further discussion.

}

// otherwise use the the cached value
return readCachedFn.readCachedValue();

This comment has been minimized.

Copy link
@puzpuzpuz

puzpuzpuz Oct 17, 2019

Contributor

If I'm reading it correctly, we'll have autoboxing here. Thus some litter per each read. Can we avoid it by introducing specialized primitive classes/methods? Or I'm mistaken and it's not the case?

This comment has been minimized.

Copy link
@blazember

blazember Oct 17, 2019

Author Contributor

Right, we autobox here. I'll think about it if we can avoid autoboxing here while keeping the logic shared. I don't worry too much about it since we have only a few gauges in the production code. Autoboxing here should be unnoticeable. Anyways, added to my improvement points list :)

blazember added 2 commits Oct 15, 2019
This change adds dynamic metric support for gauges. Prior to this change
gauges were updated from static metrics only.

Dynamic metrics are supported by introducing MetricValueCatcher that
catches the values of the dynamic metrics during the collection cycle.

The dynamic metrics provided as concrete values by calling
`collect(MetricTagger, String, ProbeLevel, ProbeUnit, long)` or
`collect(MetricTagger, String, ProbeLevel, ProbeUnit, double)` on
`MetricsCollectionContext` are cached by values, so the `gauge.read()`
calls return the last updated value for the metric.

The dynamic metrics provided through collected objects by calling
`MetricsCollectionContext#collect(MetricTagger, Object)` are cached by
source object. Once the given metric source object is discovered in a
collection cycle, the object gets cached inside the gauge and every
`gauge#read()` returns the most current value of the metric. This
caching doesn't prevent the source object to get GCd since the gauges
reference it through `WeakReference`s.

Besides allowing the metric source object to get GCd, the gauges that
are not "visited" during a metric collection cycle get reset therefore
no stale values are read once the backing dynamic  metrics sources
disappear. This is true even if the metric source object is still alive.

Fixes #15718
@blazember blazember force-pushed the blazember:4.0/fix/gh-15718 branch from e1dc17a to 31fd8d5 Oct 18, 2019
@blazember

This comment has been minimized.

Copy link
Contributor Author

blazember commented Oct 18, 2019

force-push: rebased on master

@blazember

This comment has been minimized.

Copy link
Contributor Author

blazember commented Oct 24, 2019

Needs an EE counterpart to fix the issue hazelcast/hazelcast-enterprise#3216

@petrpleshachkov petrpleshachkov self-requested a review Oct 24, 2019
class LongGaugeImpl extends AbstractGauge implements LongGauge {

private static final long DEFAULT_VALUE = 0;
static final long DEFAULT_VALUE = 0;

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 24, 2019

Contributor

Minor: 0L

This comment has been minimized.

Copy link
@blazember

blazember Oct 24, 2019

Author Contributor

👍 Done.

assertEquals(42.42D, doubleGauge.read(), 10E-6);

metricsProvider.someObject = null;
System.gc();

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 24, 2019

Contributor

It seems this call is just a hint for JVM to run GC and I am not sure that we should rely on this. We may get flaky test here.

This comment has been minimized.

Copy link
@blazember

blazember Oct 24, 2019

Author Contributor

Yeah, it's not guaranteed. I looked for other tests doing the same and it seems we call System.gc() in a few other tests too. I keep this for now as is, we can revise these calls in a separate issue if we have a test failure.

}
}

// otherwise use the the cached value

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 24, 2019

Contributor

typo "the the"

This comment has been minimized.

Copy link
@blazember

blazember Oct 24, 2019

Author Contributor

👍 Done

@blazember

This comment has been minimized.

Copy link
Contributor Author

blazember commented Oct 24, 2019

Thanks a lot for the review @petrpleshachkov @puzpuzpuz 👍

@blazember blazember merged commit e365d94 into hazelcast:master Oct 24, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@blazember blazember deleted the blazember:4.0/fix/gh-15718 branch Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.