Add new twitter event scriber impl #58

dabaitu · 2016-12-02T02:09:45Z

Part 3 of 3 to upgrade to oss 0.157

Yaliang · 2016-12-02T02:32:11Z

👍
Verified on devel.

billonahill · 2016-12-02T18:27:16Z

...entlistener-plugin/src/main/java/com/twitter/presto/plugin/eventlistener/TwitterScriber.java

+        Base64.getEncoder().encodeToString(serializer.get().serialize(thriftMessage)));
+    }
+    catch (TException e) {
+      log.warn(e, "Could not serialize thrift object" + thriftMessage);


space after object

Does TBase have a human readable toString?

There's a json serializer, but I suspect that might encounter the same exception causing us to log it. I'm thinking of just not include the thriftMessage in the log, just have the exception.

I recommend logging a few key fields from thriftMessage that might be helpful when diagnosing exceptions.

billonahill · 2016-12-02T18:30:52Z

...entlistener-plugin/src/main/java/com/twitter/presto/plugin/eventlistener/TwitterScriber.java

+    }
+  }
+
+  protected boolean scribe(Optional<String> message)


The return type is whether the message.isPresent(), which the caller already knows could this return void?

Also would it make sense to change this to take a String message and have the caller not make the call if it doesn't have one?

billonahill · 2016-12-02T18:32:34Z

...plugin/src/main/java/com/twitter/presto/plugin/eventlistener/QueryCompletedEventScriber.java

+/**
+ * Class that scribes query completion events
+ */
+public class QueryCompletedEventScriber extends TwitterScriber


Typically we want to favor composition over inheritance. What's the reasoning for using inheritance here? Seems like composition would work fine here.

You are right. The case for inheritance here is very weak, or bad. I was thinking QueryCompletedEventScriber is a TwitterScriber, but I wasn't thinking that the child is suppose to be a more limited version of the parent, which makes most sense to use composition. Will change.

billonahill · 2016-12-02T18:37:50Z

...tener-plugin/src/main/java/com/twitter/presto/plugin/eventlistener/TwitterEventListener.java

+  @Override
+  public void queryCompleted(QueryCompletedEvent queryCompletedEvent)
+  {
+    log.info(Stringify.toString(queryCompletedEvent));


There's a lot of code here to log the metadata that is mostly available to us in the UI for a given query. Do you expect we'll need to look up all of this info in the logs?

I don't. There's the raw json available in the new UIs too that has everything. For history we would look in the scribed dataset. How about I get rid of logging altogether?

👍 to get rid of the detailed logging. Maybe we instead log a few key fields that might be useful?

earlier in the thrift serialization part logging a few key fields would tell us which query completion event failed to be serialized which is useful. Logging here is actually completely redundant because both the UI and scribe log have just as much info if not more, so I can't think of a reason we will ever look in the log for query specific info. Let's get rid of it! :)

billonahill · 2016-12-02T18:42:55Z

...tener-plugin/src/main/java/com/twitter/presto/plugin/eventlistener/TwitterEventListener.java

+  public void queryCompleted(QueryCompletedEvent queryCompletedEvent)
+  {
+    log.info(Stringify.toString(queryCompletedEvent));
+    scriber.handle(queryCompletedEvent);


can we scribe before logging so if there is an exception in the toString logic it won't effect the scribe?

billonahill · 2016-12-02T18:47:33Z

...er-eventlistener-plugin/src/main/java/com/twitter/presto/plugin/eventlistener/Stringify.java

+{
+  private static final String DASH = "-";
+  private static final String FIELD_DELIMITER = " ,";
+  private static final String OBJECT_DELIMITER = "\n\t";


inserting line breaks in logging messages is typically ill advised. It makes searching and grepping difficult because you don't get the entire log line. It also can potentially break log parses that expect messages of '[timestamp] [message]' for instance.

billonahill · 2016-12-02T18:49:45Z

...er-eventlistener-plugin/src/main/java/com/twitter/presto/plugin/eventlistener/Stringify.java

+                        "principal(%s)",
+                        "remoteClientAddress(%s)"};
+    return String.format(
+      String.format("QueryContext: %s", String.join(FIELD_DELIMITER, content)),


By dynamically generating the String that's passed to String.format, we forgo the compile time checks that the format of the string matches the type and number of args passed, which is a common bug. I'd recommend not using an array to generate it dynamically and just inline it. It doesn't seem worth it just to share a comma as a delimiter.

billonahill · 2016-12-02T18:50:34Z

...er-eventlistener-plugin/src/main/java/com/twitter/presto/plugin/eventlistener/Stringify.java

+    return String.format("\nQueryCreatedEvent:\n\t%s\n", String.join(OBJECT_DELIMITER, content));
+  }
+
+  public static String toString(SplitCompletedEvent o)


if this necessary? why not just not log it.

billonahill · 2016-12-05T17:06:35Z

@dabaitu can you back out the mvn version upgrade? We should have separate PRs for upgrades that do not include feature changes.

billonahill

we should also revert the pom changes.

billonahill · 2016-12-05T17:14:43Z

...entlistener-plugin/src/main/java/com/twitter/presto/plugin/eventlistener/TwitterScriber.java

+  * Serialize a thrift object to bytes, compress, then encode as a base64 string.
+  * Throws TException
+  */
+  public String serializeThriftToString(TBase thriftMessage) throws TException


can this method be private?

billonahill · 2016-12-05T17:14:52Z

...entlistener-plugin/src/main/java/com/twitter/presto/plugin/eventlistener/TwitterScriber.java

+    }
+  }
+
+  public void scribe(String message)


can this method be private?

billonahill · 2016-12-05T17:15:48Z

...plugin/src/main/java/com/twitter/presto/plugin/eventlistener/QueryCompletedEventScriber.java

+ */
+public class QueryCompletedEventScriber
+{
+  private static String dash = "-";


sounds be final and all caps.

billonahill · 2016-12-05T17:17:37Z

...entlistener-plugin/src/main/java/com/twitter/presto/plugin/eventlistener/TwitterScriber.java

+    return Base64.getEncoder().encodeToString(serializer.get().serialize(thriftMessage));
+  }
+
+  public void scribe(TBase thriftMessage, String origEventIdentifier)


Instead of passing the exception log info in the signature, it would be cleaner to just have scribe throw TException and have QueryCompletedEventScriber catch it and handle the logging.

billonahill · 2016-12-05T20:22:25Z

...entlistener-plugin/src/main/java/com/twitter/presto/plugin/eventlistener/TwitterScriber.java

+  * Serialize a thrift object to bytes, compress, then encode as a base64 string.
+  * Throws TException
+  */
+  private String serializeThriftToString(TBase thriftMessage) throws TException


typically we put the constructor first before the methods, followed by public methods then private. would you shift these down please.

billonahill · 2016-12-05T21:17:37Z

twitter-eventlistener-plugin/src/main/java/com/twitter/presto/plugin/eventlistener/:q

@@ -0,0 +1,78 @@
+/*


accidental commit?

:(, removed.

Yaliang

Should remove "twitter-eventlistener-plugin/src/main/java/com/twitter/presto/plugin/eventlistener/:q"

billonahill

👍

dabaitu · 2016-12-06T00:46:40Z

verified all tests pass, and verified in devel with various different queries and checked twitter event listener is scribing the queries.

Thomas Sun added 2 commits December 1, 2016 18:06

add new twitter event scriber impl as plugin

c6fe499

add twitter-eventlistener-plugin as module to top level pom

978c186

billonahill reviewed Dec 2, 2016

View reviewed changes

address Bill's comments

3df6fed

billonahill requested changes Dec 5, 2016

View reviewed changes

back out version change, address comments

86917ea

billonahill reviewed Dec 5, 2016

View reviewed changes

constructor first then public, private methods

1d36d64

billonahill reviewed Dec 5, 2016

View reviewed changes

Yaliang reviewed Dec 5, 2016

View reviewed changes

remove accidental commit

41a0ab3

billonahill approved these changes Dec 5, 2016

View reviewed changes

dabaitu merged commit a286397 into twitter-forks:twitter-master Dec 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new twitter event scriber impl #58

Add new twitter event scriber impl #58

dabaitu commented Dec 2, 2016

Yaliang commented Dec 2, 2016

billonahill Dec 2, 2016

dabaitu Dec 2, 2016

billonahill Dec 2, 2016

billonahill Dec 2, 2016

billonahill Dec 2, 2016

dabaitu Dec 2, 2016

billonahill Dec 2, 2016

dabaitu Dec 2, 2016

billonahill Dec 2, 2016

dabaitu Dec 3, 2016

billonahill Dec 2, 2016

billonahill Dec 2, 2016

billonahill Dec 2, 2016

billonahill Dec 2, 2016 •

edited

Loading

billonahill commented Dec 5, 2016

billonahill left a comment

billonahill Dec 5, 2016

billonahill Dec 5, 2016

billonahill Dec 5, 2016

billonahill Dec 5, 2016

billonahill Dec 5, 2016

billonahill Dec 5, 2016

dabaitu Dec 5, 2016

Yaliang left a comment

billonahill left a comment

dabaitu commented Dec 6, 2016

Add new twitter event scriber impl #58

Add new twitter event scriber impl #58

Conversation

dabaitu commented Dec 2, 2016

Yaliang commented Dec 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billonahill Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

billonahill commented Dec 5, 2016

billonahill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yaliang left a comment

Choose a reason for hiding this comment

billonahill left a comment

Choose a reason for hiding this comment

dabaitu commented Dec 6, 2016

billonahill Dec 2, 2016 •

edited

Loading