Skip to content

Commit

Permalink
Fix some timers.
Browse files Browse the repository at this point in the history
--We forgot to stop the `DataApiRequest` timer.

--When timing the posting of the Druid Query we
failed to account for the thread hopping during
asynchronous programming. As a result, not only
were we failing to stop the timer in the correct
instance of the RequestLog (the one that is used
by the response handling thread), but we also
attempted to stop a timer in a RequestLog that
has since been cleared. So we end up with two
warnings: One for attempting to stop a timer that
doesn't exist, and one for logging a timer that
was never stopped.

Since there isn't a good way to time just the
posting of the druid query, and it probably
isn't necessary anyway, the timing has been
removed.
  • Loading branch information
Andrew Cholewa committed Jan 18, 2017
1 parent 3336ae6 commit dfda64d
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 42 deletions.
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ Current

### Changed:

- [The druid query posting timer has been removed](https://github.com/yahoo/fili/pull/141)
* There wasn't really a good way of stopping timing only the posting itself. Since the timer is
probably not that useful, it has been removed.

- [MetricMaker cleanup and simplification](https://github.com/yahoo/fili/pull/127)
* Simplified raw aggregation makers
* `ConstantMaker` now throws an `IllegalArgumentException` wrapping the raw NumberFormatException on a bad argument
Expand Down Expand Up @@ -197,6 +201,9 @@ Current

### Fixed:

- [Stopped `DataApiRequest` timer](https://github.com/yahoo/fili/pull/141):
* Stopping the `DataApiRequest` timer that we forgot to stop

- [Added missing coverage for `ThetaSketchEstimate` unwrapping in `MetricMaker.getSketchField`](https://github.com/yahoo/fili/pull/128)

- [`DataSource::getNames` now returns Fili identifiers, not fact store identifiers](https://github.com/yahoo/fili/pull/125/files)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -297,52 +297,47 @@ public void postDruidQuery(
DruidQuery<?> druidQuery
) {
long seqNum = druidQuery.getContext().getSequenceNumber();
RequestLog.startTiming("PostingDruidQuery" + seqNum);
String entityBody;
RequestLog.startTiming("DruidQuerySerializationSeq" + seqNum);
try {
String entityBody;
RequestLog.startTiming("DruidQuerySerializationSeq" + seqNum);
try {
entityBody = writer.writeValueAsString(druidQuery);
} catch (JsonProcessingException e) {
throw new IllegalStateException(e);
} finally {
RequestLog.stopTiming("DruidQuerySerializationSeq" + seqNum);
}
entityBody = writer.writeValueAsString(druidQuery);
} catch (JsonProcessingException e) {
throw new IllegalStateException(e);
} finally {
RequestLog.stopTiming("DruidQuerySerializationSeq" + seqNum);
}

long totalQueries = druidQuery.getContext().getNumberOfQueries();
String format = String.format("%%0%dd", String.valueOf(totalQueries).length());
String timerName;
AtomicLong outstanding;

if (!(druidQuery instanceof WeightEvaluationQuery)) {
if (context.getNumberOfOutgoing().decrementAndGet() == 0) {
RequestLog.stopTiming(REQUEST_WORKFLOW_TIMER);
}
outstanding = context.getNumberOfIncoming();
timerName = DRUID_QUERY_TIMER + String.format(format, seqNum);
} else {
outstanding = new AtomicLong(0);
timerName = DRUID_WEIGHTED_QUERY_TIMER + String.format(format, seqNum);
}
long totalQueries = druidQuery.getContext().getNumberOfQueries();
String format = String.format("%%0%dd", String.valueOf(totalQueries).length());
String timerName;
AtomicLong outstanding;

BoundRequestBuilder requestBuilder = webClient.preparePost(serviceConfig.getUrl())
.setBody(entityBody)
.addHeader("Content-Type", "application/json");

headersToAppend.get().forEach(requestBuilder::addHeader);

LOG.debug("druid json request: {}", entityBody);
sendRequest(
success,
error,
failure,
requestBuilder,
timerName,
outstanding
);
} finally {
RequestLog.stopTiming("PostingDruidQuery" + seqNum);
if (!(druidQuery instanceof WeightEvaluationQuery)) {
if (context.getNumberOfOutgoing().decrementAndGet() == 0) {
RequestLog.stopTiming(REQUEST_WORKFLOW_TIMER);
}
outstanding = context.getNumberOfIncoming();
timerName = DRUID_QUERY_TIMER + String.format(format, seqNum);
} else {
outstanding = new AtomicLong(0);
timerName = DRUID_WEIGHTED_QUERY_TIMER + String.format(format, seqNum);
}

BoundRequestBuilder requestBuilder = webClient.preparePost(serviceConfig.getUrl())
.setBody(entityBody)
.addHeader("Content-Type", "application/json");

headersToAppend.get().forEach(requestBuilder::addHeader);

LOG.debug("druid json request: {}", entityBody);
sendRequest(
success,
error,
failure,
requestBuilder,
timerName,
outstanding
);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,7 @@ public void getData(
uriInfo,
this
);
RequestLog.stopTiming("DataApiRequest");

if (requestMapper != null) {
apiRequest = (DataApiRequest) requestMapper.apply(apiRequest, containerRequestContext);
Expand Down

0 comments on commit dfda64d

Please sign in to comment.