Add client statistics. #6

cezarfx · 2021-08-27T14:01:18Z

Stats are collected on the client application, when system property
-Dcom.oracle.nosql.sdk.nosqldriver.stats.profile=[regular|full] is used,
for an interval of time, set by
-Dcom.oracle.nosql.sdk.nosqldriver.stats.interval=600 in seconds.

The following stats are collected for each type of request: count,
wire latencies, request and response sizes, errors, retries, retry delay,
rate limit delay. Also if rateLimiting is enabled and concurrent connections
number.

StatsConfig also provides an API to programmatically configure the statistics
collection, to selectively start and stop collection and get access to
collected statistics.

Stats are collected on the client application, when system property -Dcom.oracle.nosql.sdk.nosqldriver.stats.profile=[regular|full] is used, for an interval of time, set by -Dcom.oracle.nosql.sdk.nosqldriver.stats.interval=600 in seconds. The following stats are collected for each type of request: count, wire latencies, request and response sizes, errors, retries, retry delay, rate limit delay. Also if rateLimiting is enabled and concurent connections number. StatsConfig also provides an API to programaticaly configure the statistics collection, to selectively start and stop collection and get access to collected statistics.

Fixed throttle counter bug. Misc javadoc and clearer param and var names.

- renamed methods and fields - create full stats structure at the beginning to avoid sync locks. - switched avg calculation based on sum to be more accurate for smaller number of samples. - encapsulated writes and reads into their classes for moving sync on methods.

driver/src/main/java/oracle/nosql/driver/NoSQLHandle.java

Updated javadoc.

Adding detailed per query stats into the new profile. Renamed wireLatency to networkLatency, included Ms suffix for latency and delay entries to describe millisecond unit. Renamed reqSize and resSize to requestSize and resultSize. Add a stats log entry at end, just before shutdown. Fixed several javadoc params and added a sample output. Avoid NPE for Prepared statements without a driverPlan. Get statement from QueryRequest when available inside the prepared statement.

…tStats

Renamed wireLatency to networkLatency. Using capital L for long numerals. Fix initial message to conform to JSON.

connelly38 · 2021-10-19T23:26:05Z

driver/src/main/java/oracle/nosql/driver/NoSQLHandle.java

+    /**
+     * Returns configuration object for statistics.
+     * Statistics, when enabled, contain information like errors, delays, retries
+     * and wire latency agregated on types of requests.


typo: agregated

gmfeinberg · 2021-10-20T20:44:10Z

driver/src/main/java/oracle/nosql/driver/StatsConfig.java

+ * }
+ *     </code></p>
+ */
+public interface StatsConfig {


Why is StatsConfig an interface? It seems like an unnecessary abstraction

To hide away implementation details and avoid user distractions.

gmfeinberg · 2021-10-20T21:08:57Z

I still have some general comments:

The relationship among StatsConfig, StatsConfigImpl, Stats and how a user gets/uses/configures stats is confusing. Usually in our system a "Config" class is created essentially as a complex options object to construction of another class, in this case Stats. This is not the case here. StatsConfig is an interface, StatsConfigImpl is a "live" implementation in that changes to it at run time will (I think) affect the running stats. This is confusing and not typical. I would expect a StatsConfig object (not interface) that is used to create Stats and is not live after that. Ultimately we may want direct user control over starting/stopping/acquiring stats and how that might happen is not clear either. Maybe I'm missing something from the spec
I suggested separately to create a getName() interface on Request to avoid the calls like request.getClass().getSimpleName(). Is there a reason to not do this? It'd be simpler and more efficient. Minimally create a local method to do this (String getName(Request)). This approach would also make the mapping of request names more reliable than code like this:
String requestClass = kvRequest.getClass().getSimpleName();
requestClass = requestClass.endsWith("Request")
? requestClass.substring(0, requestClass.length() - 7) :
requestClass;
rStat = requests.get(requestClass);
New files are missing Copyright headers
Stats.java has no comments at all

connelly38 · 2021-10-20T21:14:28Z

1. The relationship among StatsConfig, StatsConfigImpl, Stats and how a user gets/uses/configures stats is confusing. Usually in our system a "Config" class is created essentially as a complex options object to construction of another class, in this case Stats. This is not the case here. StatsConfig is an interface, StatsConfigImpl is a "live" implementation in that changes to it at run time will (I think) affect the running stats. This is confusing and not typical. I would expect a StatsConfig object (not interface) that is used to create Stats and is not live after that. Ultimately we may want direct user control over starting/stopping/acquiring stats and how that might happen is not clear either. Maybe I'm missing something from the spec

After reading this again, I agree with George. StatsConfig should just be a class. No need for an Impl.

gmfeinberg · 2021-10-20T21:23:10Z

I'm also confused about what the extra query stats are storing. Can you talk about them? I didn't expect stats per statement that would include the statement and driver side query plan

cezarfx · 2021-10-21T17:43:23Z

I'm also confused about what the extra query stats are storing. Can you talk about them? I didn't expect stats per statement that would include the statement and driver side query plan

The query request stats are too generic when trying to find out what query is doing something significant. The extra query stats show the query stats on a per query statement base. This way certain query can be identified by checking its stats.

cezarfx · 2021-10-21T19:08:38Z

The relationship among StatsConfig, StatsConfigImpl, Stats and how a user gets/uses/configures stats is confusing. Usually in our system a "Config" class is created essentially as a complex options object to construction of another class, in this case Stats. This is not the case here. StatsConfig is an interface, StatsConfigImpl is a "live" implementation in that changes to it at run time will (I think) affect the running stats. This is confusing and not typical. I would expect a StatsConfig object (not interface) that is used to create Stats and is not live after that. Ultimately we may want direct user control over starting/stopping/acquiring stats and how that might happen is not clear either. Maybe I'm missing something from the spec

StatsConfig is the interface that allows users to enable/disable the collection of stats and set which stats to collect. It is live because users can start/stop or change collection at anytime, useful for debugging hard to reproduce or transitory issues.

I suggested separately to create a getName() interface on Request to avoid the calls like request.getClass().getSimpleName(). Is there a reason to not do this? It'd be simpler and more efficient. Minimally create a local method to do this (String getName(Request)). This approach would also make the mapping of request names more reliable than code like this:
String requestClass = kvRequest.getClass().getSimpleName();
requestClass = requestClass.endsWith("Request")
? requestClass.substring(0, requestClass.length() - 7) :
requestClass;
rStat = requests.get(requestClass);

Adding a getName() would be another way to do it.

New files are missing Copyright headers
Adding headers.

Stats.java has no comments at all
Adding comments.

…o StatsControl. Added copyright comments to new files. Renamed: count to httpRequestCount, networkLatencyMs to httpRequestLatencyMs, countAPI to count for query requests and stmt to query. Added stacktrace to log when there is an exception during stats logging. Using request.getName() for all request types.

gmfeinberg · 2021-10-28T16:53:39Z

@cezarfx There are javadoc warnings in mvn. See "mvn javadoc:javadoc"

gmfeinberg · 2021-10-28T17:01:11Z

driver/src/main/java/oracle/nosql/driver/http/StatsControlImpl.java

+public class StatsControlImpl
+    implements StatsControl {
+
+    static public final String LOG_PREFIX = "ONJS:Monitoring stats|";


Where did this prefix come from? It seems a bit awkward. Why not just something simple like "STATS" or "STATISTICS" ?

Fix javadoc errors and warnings.

Changes StatsHandler to accept MapValue instead of FieldValue. Fixed javadoc.

Fixed javadoc.

cezarfx added 3 commits August 26, 2021 18:28

Moved rateLimitingEnabled from interval log to initial log output.

6f5d0b2

Fixed throttle counter bug. Misc javadoc and clearer param and var names.

Various fixes:

d05f5d8

- renamed methods and fields - create full stats structure at the beginning to avoid sync locks. - switched avg calculation based on sum to be more accurate for smaller number of samples. - encapsulated writes and reads into their classes for moving sync on methods.

gmfeinberg requested review from connelly38 and gmfeinberg September 11, 2021 21:50

gmfeinberg reviewed Sep 11, 2021

View reviewed changes

driver/src/main/java/oracle/nosql/driver/NoSQLHandle.java Outdated Show resolved Hide resolved

cezarfx and others added 6 commits September 13, 2021 19:20

Update NoSQLHandle.java

2c2f41c

Updated javadoc.

Update StatsConfig.java

007e1bf

Updated javadoc.

Merge remote-tracking branch 'remotes/origin/main' into feature/clien…

54a421b

…tStats

Remove double getAcquiredChannelCount().

16a315a

Fix Percentile to be more exact and avoid errors for negative values.

84f5b58

Renamed wireLatency to networkLatency. Using capital L for long numerals. Fix initial message to conform to JSON.

connelly38 reviewed Oct 19, 2021

View reviewed changes

connelly38 approved these changes Oct 19, 2021

View reviewed changes

gmfeinberg reviewed Oct 20, 2021

View reviewed changes

gmfeinberg reviewed Oct 28, 2021

View reviewed changes

cezarfx added 5 commits October 28, 2021 14:05

Set use stats handler set in NoSQLHandleConfig.

6b7663b

Fix javadoc errors and warnings.

Added entry in CHANGELOG.md.

4d8e81f

Changes StatsHandler to accept MapValue instead of FieldValue. Fixed javadoc.

Added getStatsHandler in StatsControl along the setter.

e4d6089

Merge remote-tracking branch 'origin/main' into feature/clientStats

a5c0b92

Moved LOG_PREFIX to StatsControl and changed it to Client stats.

7fe4557

Fixed javadoc.

cezarfx merged commit 752c1ea into main Nov 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add client statistics. #6

Add client statistics. #6

Uh oh!

cezarfx commented Aug 27, 2021

Uh oh!

Uh oh!

connelly38 Oct 19, 2021

Uh oh!

gmfeinberg Oct 20, 2021

Uh oh!

cezarfx Oct 21, 2021

Uh oh!

gmfeinberg commented Oct 20, 2021 •

edited

Loading

Uh oh!

connelly38 commented Oct 20, 2021

Uh oh!

gmfeinberg commented Oct 20, 2021

Uh oh!

cezarfx commented Oct 21, 2021

Uh oh!

cezarfx commented Oct 21, 2021

Uh oh!

gmfeinberg commented Oct 28, 2021

Uh oh!

gmfeinberg Oct 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add client statistics. #6

Add client statistics. #6

Uh oh!

Conversation

cezarfx commented Aug 27, 2021

Uh oh!

Uh oh!

connelly38 Oct 19, 2021

Choose a reason for hiding this comment

Uh oh!

gmfeinberg Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

cezarfx Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

gmfeinberg commented Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

connelly38 commented Oct 20, 2021

Uh oh!

gmfeinberg commented Oct 20, 2021

Uh oh!

cezarfx commented Oct 21, 2021

Uh oh!

cezarfx commented Oct 21, 2021

Uh oh!

gmfeinberg commented Oct 28, 2021

Uh oh!

gmfeinberg Oct 28, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gmfeinberg commented Oct 20, 2021 •

edited

Loading