Offloaders: fix metrics #16405

eolivelli · 2022-07-05T13:46:17Z

Motivation

There are a few problems with Offloader metrics:

the scheduler does not run
JClouds read latency is not calculated well, because the whole read time must be taken after streaming the whole HTTP payload
we miss a raw counter of the amount of data read from Tiered Storage

Modifications

pass the Scheduler for periodic calculations
add raw brk_ledgeroffloader_read_bytes counter
fix read latency from JClouds calculation

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

hangc0276

Good job!

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/LedgerOffloaderStatsImpl.java

hangc0276 · 2022-07-11T05:32:15Z

ping @tjiuming @zymap @horizonzy, Please help take a look at this pr, thanks.

zymap · 2022-07-11T05:40:17Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/LedgerOffloaderStatsImpl.java

 import org.apache.bookkeeper.mledger.LedgerOffloaderStats;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.tuple.ImmutablePair;
 import org.apache.commons.lang3.tuple.Pair;
 import org.apache.pulsar.common.naming.TopicName;
-
+@Slf4j


Unused import?

good catch. removed

@zymap

- pass the Scheduler for periodic calculations - add raw brk_ledgeroffloader_read_bytes counter - fix read latency from JClouds calculation

tjiuming

LGTM

horizonzy · 2022-07-12T07:43:12Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

-                    OffloadPoliciesImpl.create(this.getConfiguration().getProperties()));
+            OffloadPoliciesImpl defaultOffloadPolicies =
+                    OffloadPoliciesImpl.create(this.getConfiguration().getProperties());
+            this.defaultOffloader = createManagedLedgerOffloader(defaultOffloadPolicies);


We should make

this.defaultOffloader = createManagedLedgerOffloader(defaultOffloadPolicies);

after

offloaderStats = LedgerOffloaderStats.create(config.isExposeManagedLedgerMetricsInPrometheus(), exposeTopicMetrics, offloaderScheduler, interval);

In createManagedLedgerOffloader, it will use offloaderStats. We shuold make this method after offloaderStats override.

eolivelli · 2022-07-13T08:59:11Z

@codelipenghui @nicoloboschi @Technoboy-

the error in the test (broker group 1) is unrelated to this patch

Error: Tests run: 43, Failures: 1, Errors: 0, Skipped: 9, Time elapsed: 51.938 s <<< FAILURE! - in org.apache.pulsar.broker.service.PersistentTopicTest
Error: setup(org.apache.pulsar.broker.service.PersistentTopicTest) Time elapsed: 5.343 s <<< FAILURE!
org.mockito.exceptions.base.MockitoException: Unable to create mock instance of type 'ServerCnx'
at org.apache.pulsar.broker.BrokerTestUtil.spyWithClassAndConstructorArgs(BrokerTestUtil.java:43)
at org.apache.pulsar.broker.service.PersistentTopicTest.setup(PersistentTopicTest.java:226)
at jdk.internal.reflect.GeneratedMethodAccessor576.invoke(Unknown Source)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:568)
at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
at org.testng.internal.MethodInvocationHelper.invokeMethodConsideringTimeout(MethodInvocationHelper.java:61)
at org.testng.internal.ConfigInvoker.invokeConfigurationMethod(ConfigInvoker.java:366)
at org.testng.internal.ConfigInvoker.invokeConfigurations(ConfigInvoker.java:320)
at org.testng.internal.TestInvoker.runConfigMethods(TestInvoker.java:701)
at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:527)
at org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174)
at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46)
at org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822)
at org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147)
at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
at org.testng.TestRunner.privateRun(TestRunner.java:764)
at org.testng.TestRunner.run(TestRunner.java:585)
at org.testng.SuiteRunner.runTest(SuiteRunner.java:384)
at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378)
at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337)
at org.testng.SuiteRunner.run(SuiteRunner.java:286)
at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53)
at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96)
at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218)
at org.testng.TestNG.runSuitesLocally(TestNG.java:1140)
at org.testng.TestNG.runSuites(TestNG.java:1069)
at org.testng.TestNG.run(TestNG.java:1037)
at org.apache.maven.surefire.testng.TestNGExecutor.run(TestNGExecutor.java:135)
at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeSingleClass(TestNGDirectoryTestSuite.java:112)
at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeLazy(TestNGDirectoryTestSuite.java:123)
at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.execute(TestNGDirectoryTestSuite.java:90)
at org.apache.maven.surefire.testng.TestNGProvider.invoke(TestNGProvider.java:146)
at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384)
at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345)
at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126)
at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)
Caused by: org.mockito.creation.instance.InstantiationException:
Unable to create instance of 'ServerCnx'.
Please ensure the target class has a constructor that matches these argument types: [org.apache.pulsar.broker.PulsarService] and executes cleanly.
... 39 more
Caused by: java.lang.reflect.InvocationTargetException
at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.newInstance(InstrumentationMemberAccessor.java:198)
at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.newInstance(InstrumentationMemberAccessor.java:161)
at org.mockito.internal.util.reflection.ModuleMemberAccessor.newInstance(ModuleMemberAccessor.java:42)
at org.mockito.internal.creation.instance.ConstructorInstantiator.invokeConstructor(ConstructorInstantiator.java:70)
at org.mockito.internal.creation.instance.ConstructorInstantiator.withParams(ConstructorInstantiator.java:53)
at org.mockito.internal.creation.instance.ConstructorInstantiator.newInstance(ConstructorInstantiator.java:39)
at org.mockito.internal.creation.bytebuddy.InlineDelegateByteBuddyMockMaker.doCreateMock(InlineDelegateByteBuddyMockMaker.java:360)
at org.mockito.internal.creation.bytebuddy.InlineDelegateByteBuddyMockMaker.createMock(InlineDelegateByteBuddyMockMaker.java:330)
at org.mockito.internal.creation.bytebuddy.InlineByteBuddyMockMaker.createMock(InlineByteBuddyMockMaker.java:58)
at org.mockito.internal.util.MockUtil.createMock(MockUtil.java:53)
at org.mockito.internal.MockitoCore.mock(MockitoCore.java:84)
at org.mockito.Mockito.mock(Mockito.java:1964)
... 39 more
Caused by: java.lang.ClassCastException: class org.apache.pulsar.broker.service.BrokerService cannot be cast to class org.apache.pulsar.broker.resources.PulsarResources (org.apache.pulsar.broker.service.BrokerService and org.apache.pulsar.broker.resources.PulsarResources are in unnamed module of loader 'app')
at org.apache.pulsar.broker.PulsarService.getPulsarResources(PulsarService.java:265)
at org.apache.pulsar.broker.service.TopicListService.(TopicListService.java:103)
at org.apache.pulsar.broker.service.ServerCnx.(ServerCnx.java:279)
at org.apache.pulsar.broker.service.ServerCnx.(ServerCnx.java:239)
at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)
at org.mockito.internal.util.reflection.InstrumentationMemberAccessor$Dispatcher$ByteBuddy$rxYoJ6WV.invokeWithArguments(Unknown Source)
at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.lambda$newInstance$0(InstrumentationMemberAccessor.java:191)
at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.newInstance(InstrumentationMemberAccessor.java:188)
... 50 more

eolivelli · 2022-07-13T13:31:48Z

/pulsarbot rerun-failure-checks

eolivelli added area/tieredstorage area/metrics doc-not-needed Your PR changes do not impact docs labels Jul 5, 2022

eolivelli self-assigned this Jul 5, 2022

eolivelli requested a review from hangc0276 July 5, 2022 13:46

nicoloboschi approved these changes Jul 6, 2022

View reviewed changes

eolivelli closed this Jul 6, 2022

eolivelli reopened this Jul 6, 2022

nicoloboschi mentioned this pull request Jul 6, 2022

[fix][workflow] Cancel duplicate docbot workflows #16368

Merged

5 tasks

hangc0276 approved these changes Jul 6, 2022

View reviewed changes

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/LedgerOffloaderStatsImpl.java Show resolved Hide resolved

eolivelli closed this Jul 7, 2022

eolivelli reopened this Jul 7, 2022

eolivelli added doc-label-missing and removed doc-label-missing labels Jul 7, 2022

zymap reviewed Jul 11, 2022

View reviewed changes

eolivelli added 5 commits July 11, 2022 14:03

Offloaders: fix metrics

c34c24f

- pass the Scheduler for periodic calculations - add raw brk_ledgeroffloader_read_bytes counter - fix read latency from JClouds calculation

checkstyle

0284e1c

style

fc475ef

Add 1.0 quantile

10edac3

address comment

430ec7a

eolivelli force-pushed the fix/offloader-metrics branch from caf6868 to 430ec7a Compare July 11, 2022 12:03

tjiuming approved these changes Jul 11, 2022

View reviewed changes

zymap approved these changes Jul 11, 2022

View reviewed changes

horizonzy reviewed Jul 12, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into fix/offloader-metrics

5d64790

eolivelli merged commit ac3f3ee into apache:master Jul 14, 2022

wuxuanqicn pushed a commit to wuxuanqicn/pulsar that referenced this pull request Jul 14, 2022

Offloaders: fix metrics (apache#16405)

1707fd9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Offloaders: fix metrics #16405

Offloaders: fix metrics #16405

eolivelli commented Jul 5, 2022

hangc0276 left a comment

hangc0276 commented Jul 11, 2022

zymap Jul 11, 2022

eolivelli Jul 11, 2022

tjiuming left a comment

horizonzy Jul 12, 2022

eolivelli commented Jul 13, 2022

eolivelli commented Jul 13, 2022

Offloaders: fix metrics #16405

Offloaders: fix metrics #16405

Conversation

eolivelli commented Jul 5, 2022

Motivation

Modifications

Verifying this change

hangc0276 left a comment

Choose a reason for hiding this comment

hangc0276 commented Jul 11, 2022

zymap Jul 11, 2022

Choose a reason for hiding this comment

eolivelli Jul 11, 2022

Choose a reason for hiding this comment

tjiuming left a comment

Choose a reason for hiding this comment

horizonzy Jul 12, 2022

Choose a reason for hiding this comment

eolivelli commented Jul 13, 2022

eolivelli commented Jul 13, 2022