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

Dynamic metrics enhancement and cleanup #15779

Merged

Conversation

@blazember
Copy link
Contributor

blazember commented Oct 15, 2019

Enhancements and cleanups related to dynamic metrics collection needed by Jet.

  • Rename MetricsExtractor to MetricsCollectionContext
  • Add methods to MetricsCollectionContext:
    • collect(MetricTagger, String, ProbeLevel, ProbeUnit, long)
    • collect(MetricTagger, String, ProbeLevel, ProbeUnit, double)
  • Remove methods that register metrics from MetricTagger
  • Registration methods removed from MetricTagger added to MetricsRegistry

EE PR: hazelcast/hazelcast-enterprise#3241

@blazember blazember added this to the 4.0 milestone Oct 15, 2019
@blazember blazember requested review from cangencer and viliam-durina Oct 15, 2019
@blazember blazember self-assigned this Oct 15, 2019
@blazember blazember force-pushed the blazember:4.0/mc-metrics/dynamic-metrics-cleanup branch from 9189203 to 4092546 Oct 15, 2019
@blazember blazember force-pushed the blazember:4.0/mc-metrics/dynamic-metrics-cleanup branch from 4092546 to 552b97b Oct 16, 2019
Remove `MetricsRegistry` dependency from `MetricTaggerImpl`.
Add `MetricTagger getMetricTagger()` to `MetricTaggerSupplier`.
@puzpuzpuz puzpuzpuz self-requested a review Oct 16, 2019
Copy link
Contributor

puzpuzpuz left a comment

Looks really nice. Left a couple of suggestions for minor improvements.

* @param function the probe function
* @throws NullPointerException if source, name, level or probe is null.
*/
<S> void registerStaticProbe(S source, MetricTagger tagger, String name, ProbeLevel level, ProbeUnit unit,

This comment has been minimized.

Copy link
@puzpuzpuz

puzpuzpuz Oct 16, 2019

Contributor

tagger param is missing in the javadoc for this method.

* @param probe the probe
* @throws NullPointerException if source, name, level or probe is null.
*/
<S> void registerStaticProbe(S source, MetricTagger tagger, String name, ProbeLevel level, ProbeUnit unit,

This comment has been minimized.

Copy link
@puzpuzpuz

puzpuzpuz Oct 16, 2019

Contributor

Same javadoc problem with tagger

* @param probe the probe
* @throws NullPointerException if source, name, level or probe is null.
*/
<S> void registerStaticProbe(S source, MetricTagger tagger, String name, ProbeLevel level, ProbeUnit unit,

This comment has been minimized.

Copy link
@puzpuzpuz

puzpuzpuz Oct 16, 2019

Contributor

Same javadoc problem with tagger


private <S> void registerStaticProbeWithUnit(S source, MetricTagger tagger, String name, ProbeLevel level, ProbeUnit unit,
ProbeFunction function) {
registerInternal(source, tagger.withTag("unit", unit.name().toLowerCase())

This comment has been minimized.

Copy link
@puzpuzpuz

puzpuzpuz Oct 16, 2019

Contributor

Shouldn't this method include null checks similar to #registerStaticProbeWithoutUnit?

@@ -19,8 +19,6 @@
import com.hazelcast.internal.metrics.collectors.MetricsCollector;
import com.hazelcast.internal.metrics.impl.MetricsRegistryImpl;

import javax.annotation.Nonnull;

/**
* Interface to be used for tagging a metric.

This comment has been minimized.

Copy link
@puzpuzpuz

puzpuzpuz Oct 16, 2019

Contributor

Does it make sense to introduce a separate #withUnitTag(ProbeUnit) method? I can see lots of duplicate .withTag("unit", unit.name().toLowerCase()) calls. We could make them less error-prone by introducing that special tag method.

This comment has been minimized.

Copy link
@blazember

blazember Oct 16, 2019

Author Contributor

Yes, making unit a first-class citizen definitely makes sense. For the reason you mentioned. I'll address it in a separate PR.

@blazember

This comment has been minimized.

Copy link
Contributor Author

blazember commented Oct 16, 2019

Thank you a lot for the review and the insights @cangencer @puzpuzpuz, really appreciated.

@puzpuzpuz I address the minors you commented in a separate PR and merging this, so Can can progress.

@blazember blazember merged commit 13acc78 into hazelcast:master Oct 16, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@blazember blazember deleted the blazember:4.0/mc-metrics/dynamic-metrics-cleanup branch Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.