ResponseException stores the JSON sent to Druid. #70
Conversation
archolewa
force-pushed
the
actually-log-druid-query
branch
from
52883ad to
7206352
Compare
| @@ -37,6 +45,29 @@ public ResponseException(Response.StatusType statusType, DruidAggregationQuery<? | |||
| } | |||
|
|
|||
| /** | |||
| * Class constructor with throwable, other parameters and a mapper for serializing the druid query. | |||
| * @param statusType Status type of the response | |||
| @@ -64,7 +95,29 @@ public ResponseException( | |||
| DruidQuery<?> druidQuery, | |||
| Throwable cause | |||
| ) { | |||
| super(buildMessage(reason, description, statusCode, druidQuery, cause)); | |||
| this(statusCode, reason, description, druidQuery, cause, new ObjectMapper().writer()); | |||
| } | |||
There was a problem hiding this comment.
We should deprecate this constructor, since creating a new "naked" mapper w/o the extensions we have on the primary ones will result in a funky serialization.
There was a problem hiding this comment.
And by that token, probably deprecate other constructors to this thing that don't take in the writer they'll need as well.
There was a problem hiding this comment.
Oh, and also not increase the number of deprecation warnings the compiler throws at us if we can 😉
archolewa
force-pushed
the
actually-log-druid-query
branch
from
7adaf34 to
cbb204e
Compare
| druidQueryString = objectWriter.writeValueAsString(druidQuery); | ||
| } catch (JsonProcessingException e) { | ||
| druidQueryString = druidQuery.toString(); | ||
| LOG.warn(String.format("Failed to properly serialize druid query %s", druidQueryString), e); |
There was a problem hiding this comment.
Flip these. Otherwise, if the toString() blows up, we never see the next log message
There was a problem hiding this comment.
If I do that, then the best we can say is "Failed to properly serialize a druid query." without any sort of string representation. Which admittedly isn't going to be that different from what will be displayed now, since we don't override toString.
There was a problem hiding this comment.
Ah, makes sense. With that in mind, you can actually just in-line druidQuery in the LOG call, and let the logging framework call toString if it decides it needs to actually log it.
archolewa
force-pushed
the
actually-log-druid-query
branch
2 times, most recently
from
1bf0d94 to
95f4dea
Compare
|
👍 |
-- `DruidAggregationQuery` does not have `toString` defined. Furthermore, it would be non-trivial to define a `toString` that serializes the query into the actual query sent to Druid (we would need to inject a `ObjectMapper` from the `ObjectMapperSuite` into the aggregation query). Therefore, the `ResponseException` now takes an `ObjectWriter` that can be used to serialize the druid query that is logged.
archolewa
force-pushed
the
actually-log-druid-query
branch
from
95f4dea to
aac87bc
Compare
--
DruidAggregationQuerydoes not havetoStringdefined.Furthermore, it would be non-trivial to define a
toStringthatserializes the query into the actual query sent to Druid (we would need
to inject a
ObjectMapperfrom theObjectMapperSuiteinto theaggregation query). Therefore, the
ResponseExceptionnow takes anObjectWriterthat can be used to serialize the druid query that islogged.