Framework for sending emails #135

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

jerryli9876 commented Aug 30, 2012

  • Switched from hourly to daily preprocessors
  • Modified scripts to expect daily data
  • Added Email sending capabilities
  • Added Email formatting using Mustache

@johanoskarsson johanoskarsson commented on an outdated diff Aug 30, 2012

...ala/com/twitter/zipkin/hadoop/WriteToFileClient.scala
if (line.getValue().length < 2) {
throw new IllegalArgumentException("Malformed line: " + line)
}
val hypertext = line.getValue().head
- (Util.ZIPKIN_TRACE_URL + hypertext, hypertext, line)
+ val duration = augmentString(line.getValue().tail.head).toFloat
+ if (duration > 1000) {
+ val inMilliseconds = scala.math.round(duration * 100 / 1000.0) / 100.0
+ val formatted = new PerServiceLineResult((List(line.getKey(), hypertext, inMilliseconds.toString)))
+ Some((Util.ZIPKIN_TRACE_URL + hypertext, hypertext, formatted))
@johanoskarsson

johanoskarsson Aug 30, 2012

Contributor

ZIPKIN_TRACE_URL should be a setting and not a constant. Must have missed that in another branch.

@johanoskarsson johanoskarsson commented on an outdated diff Aug 30, 2012

...ala/com/twitter/zipkin/hadoop/WriteToFileClient.scala
@@ -164,6 +170,20 @@ class ExpensiveEndpointsClient extends WriteToTableClient("ExpensiveEndpoints")
}
def getTableHeader() = {
- List("Service Called", "Duration")
+ List("Service Called", "Duration (ms)")
+ }
+
+ override def addTable(service: String, lines: List[LineResult], mt: EmailContent) = {
+ val formatted = lines.map {line =>
+ if (line.getValue().length < 2) {
+ throw new IllegalArgumentException("Malformed line: " + line)
+ }
+ val service = line.getValue().head
+ val duration = augmentString(line.getValue().tail.head).toFloat
+ val rounded = scala.math.round(duration * 100) / 100.0
+ println(rounded)
@johanoskarsson

johanoskarsson Aug 30, 2012

Contributor

Let's change all the println to proper logging statements

@johanoskarsson johanoskarsson commented on the diff Aug 30, 2012

...ain/scala/com/twitter/zipkin/hadoop/email/Email.scala
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+package com.twitter.zipkin.hadoop.email
+
+import javax.mail._
+import javax.mail.internet.{InternetAddress, MimeMessage}
+import javax.mail.Message.RecipientType
+
+/**
+* An Email is a class that represents a single email (including text) and a single SMTP session, and which allows you
@johanoskarsson

johanoskarsson Aug 30, 2012

Contributor

Wouldn't it make more sense to split out the client and the email into two classes? Create an instance of the client and send multiple emails to it.

@johanoskarsson johanoskarsson commented on an outdated diff Aug 30, 2012

zipkin-hadoop/src/scripts/run_all_jobs.rb
@@ -97,9 +96,15 @@ def run_all_jobs(dates, output, hadoop_config)
JobArguments.new(dates, "\" --error_type finagle.timeout \"", config_string, "UTC", "Timeouts", output + "/Timeouts", false),
JobArguments.new(dates, " \" --error_type finagle.retry \"", config_string, "UTC", "Timeouts", output + "/Retries", false),]
+<<<<<<< HEAD
@johanoskarsson

johanoskarsson Aug 30, 2012

Contributor

Some botched merge here

Contributor

johanoskarsson commented Aug 30, 2012

This stuff is complex enough to warrant a README describing how it works, what configs to change and so on.

@johanoskarsson johanoskarsson commented on the diff Aug 30, 2012

...la/com/twitter/zipkin/hadoop/email/EmailContent.scala
def writeAll() = {
for (service <- services()) {
- val pw = new PrintWriter(new FileOutputStream(serviceToHtml(service), true))
- templates(service).write(pw)
+ if (templates.contains(service)) {
+ val pw = new PrintWriter(new FileOutputStream(outputDir + "/" + Util.toSafeHtmlName(service), true))
@johanoskarsson

johanoskarsson Aug 30, 2012

Contributor

Should be closed

@franklinhu

franklinhu Aug 30, 2012

Contributor

When you close it should also wrap it in a try{...} finally{<close here>} to make sure it closes even if you have exceptions

@johanoskarsson johanoskarsson commented on an outdated diff Aug 30, 2012

zipkin-hadoop-job-runner/src/scripts/has-sampled.rb
@@ -1,17 +1,4 @@
#!/usr/bin/env ruby
-# Copyright 2012 Twitter Inc.
@johanoskarsson

johanoskarsson Aug 30, 2012

Contributor

We should keep the license.

@johanoskarsson johanoskarsson commented on an outdated diff Aug 30, 2012

...la/com/twitter/zipkin/hadoop/ExpensiveEndpoints.scala
.write(Tsv(args("output")))
}
+
+object ExpensiveEndpoints {
+ val THRESHOLD = 0.001
@johanoskarsson

johanoskarsson Aug 30, 2012

Contributor

Comment describing this in more detail would be nice. What's the unit?

@johanoskarsson johanoskarsson commented on the diff Aug 30, 2012

.../com/twitter/zipkin/hadoop/sources/Preprocessed.scala
@@ -43,7 +43,11 @@ class Preprocessed(args : Args) extends Job(args) with DefaultDateRangeJob {
a : (Long, Long, Long, List[Annotation], List[BinaryAnnotation]) =>
a match {
case (tid, id, pid, annotations, binary_annotations) =>
- new gen.Span(tid, "", id, annotations.asJava, binary_annotations.asJava).setParent_id(pid)
+ val span = new gen.Span(tid, "", id, annotations.asJava, binary_annotations.asJava)
+ if (pid != 0) {
@johanoskarsson

johanoskarsson Aug 30, 2012

Contributor

Technically we can get parent ids that are 0, even though it's not all that likely. If it's possible to use an option here that would be preferable.

@jerryli9876

jerryli9876 Aug 30, 2012

Contributor

AFAIK, the only way to do that is to retain the original span throughout all the processing, and that's already super slow as it is.

@johanoskarsson

johanoskarsson Aug 31, 2012

Contributor

Since we're running out of time, let's just put a TODO here in that case describing the issue.

@franklinhu franklinhu commented on an outdated diff Aug 30, 2012

...main/scala/com/twitter/zipkin/hadoop/LineResult.scala
@@ -77,7 +79,7 @@ class PerServiceLineResult(line: List[String]) extends LineResult(line) {
}
def getKey() = {
- line.head
+ line.head
@franklinhu

franklinhu Aug 30, 2012

Contributor

whitespace

@franklinhu franklinhu commented on an outdated diff Aug 30, 2012

...ain/scala/com/twitter/zipkin/hadoop/email/Email.scala
+ sessionProps.put("mail.smtp.port", smtpPort.asInstanceOf[AnyRef])
+ sessionProps.put("mail.smtp.auth", smtpAuth.asInstanceOf[AnyRef])
+
+
+ /**
+ * Sends a message with the given subject and body to the specified to list.
+ *
+ * @param to non-null list of addresses
+ * @param subject non-null subject line
+ * @param body non-null body of the message
+ */
+ def send(to: String, subject: String, body: String): Boolean = {
+ send(Map(RecipientType.TO -> to), subject, body)
+ }
+
+ def send(_toMap: Map[RecipientType, String], _subject: String, _body: String): Boolean = {
@franklinhu

franklinhu Aug 30, 2012

Contributor

Variable names prefixed with _ make me feel kinda dirty when I use them.
It might be cleaner to just subclass Email with TestEmail and and construct the right one in MailConfig. TestEmail can then just override the send method use the right defaults and call the super method.

@franklinhu franklinhu commented on an outdated diff Aug 30, 2012

...la/com/twitter/zipkin/hadoop/email/EmailContent.scala
private var document = List[Html]()
def html() = {
document.asJava
}
- case class Html(var header: java.util.List[Header], var body: java.util.List[Body] ) {}
+ case class Html(var header: java.util.List[Header], var services: java.util.List[Service] ) {}
@franklinhu

franklinhu Aug 30, 2012

Contributor

You can ignore this one, but if you want to avoid having java.util.List everywhere you can do import java.util.{List => JList} then just use JList everywhere

@franklinhu franklinhu commented on an outdated diff Aug 30, 2012

...la/com/twitter/zipkin/hadoop/email/EmailContent.scala
+ }
+ }
+
+ /**
+ * Writes all the email contents to Strings
+ * @return a Map from service name to the email contents as a String
+ */
+ def writeAllAsStrings() = {
+ var serviceToEmail = Map[String, String]()
+ for (service <- services()) {
+ val sw = new StringWriter()
+ val pw = new PrintWriter(sw)
+ if (templates.contains(service)) {
+ templates(service).write(pw)
+ serviceToEmail += service -> sw.toString()
+ }
@franklinhu

franklinhu Aug 30, 2012

Contributor

This one should be closed too

@franklinhu franklinhu and 1 other commented on an outdated diff Aug 30, 2012

...cala/com/twitter/zipkin/hadoop/email/MailConfig.scala
+ var bcc: String = "zipkin-service-report@abc.efg"
+ var auth: Boolean = true
+ var user: String = "username"
+ var password: String = "password"
+
+ // All test emails are delivered to this address
+ var testTo: String = "zipkin-service-report-test@abc.efg"
+
+ def apply(testMode: Boolean): Email = {
+ new Email(from, testTo, bcc, testMode, smtpServer, smtpPort, auth, user, password)
+ }
+
+ def apply(): Email = {
+ apply(false)
+ }
+}
@franklinhu

franklinhu Aug 30, 2012

Contributor

You can just make it a default param:

def apply(testMode: Boolean = false)
@jerryli9876

jerryli9876 Aug 30, 2012

Contributor

Config expects an apply() method without any parameters.

@johanoskarsson johanoskarsson commented on an outdated diff Aug 31, 2012

...ain/scala/com/twitter/zipkin/hadoop/Postprocess.scala
*/
object PostprocessWriteToFile {
- val jobList = List(("WorstRuntimesPerTrace", new WorstRuntimesPerTraceClient(Util.ZIPKIN_TRACE_URL)),
+ val jobList = List(("WorstRuntimesPerTrace", (new WorstRuntimesPerTraceClientConfig()).apply()),
@johanoskarsson

johanoskarsson Aug 31, 2012

Contributor

No big deal, but you can call apply by just adding (). So instead of .apply() just do ().

@johanoskarsson johanoskarsson commented on an outdated diff Aug 31, 2012

...hadoop/config/WorstRuntimesPerTraceClientConfig.scala
@@ -0,0 +1,22 @@
+package com.twitter.zipkin.hadoop.config
+
+import com.twitter.util.Config
+import com.twitter.zipkin.hadoop.WorstRuntimesPerTraceClient
+
+/**
@johanoskarsson

johanoskarsson Aug 31, 2012

Contributor

Remove this and add license.

@johanoskarsson johanoskarsson commented on an outdated diff Aug 31, 2012

...hadoop/config/WorstRuntimesPerTraceClientConfig.scala
+package com.twitter.zipkin.hadoop.config
+
+import com.twitter.util.Config
+import com.twitter.zipkin.hadoop.WorstRuntimesPerTraceClient
+
+/**
+ * Created with IntelliJ IDEA.
+ * User: jli
+ * Date: 8/30/12
+ * Time: 4:07 PM
+ * To change this template use File | Settings | File Templates.
+ */
+
+class WorstRuntimesPerTraceClientConfig extends Config[WorstRuntimesPerTraceClient] {
+
+ val zipkinTraceUrl = "your.zipkin.url/traces"
@johanoskarsson

johanoskarsson Aug 31, 2012

Contributor

Should be a var

Contributor

johanoskarsson commented Aug 31, 2012

+1 except for the above

@adriancole adriancole added a commit that referenced this pull request Jun 3, 2016

@adriancole adriancole Makes AsyncSpanConsumer the authoritative type for receiving spans
Before, `accept(List<Span>)` was on `SpanStore`, a blocking interface.
This helped for a while, but it is clear storage commands should be
async, particularly for reasons listed in #133.

This change removes writes from `SpanStore` and re-organizes the code
around `AsyncSpanConsumer`. This is the first refactor on the way
towards a component model #135.
dd90469

@adriancole adriancole added a commit that referenced this pull request Jun 3, 2016

@adriancole adriancole Renames storage artifacts from spanstore-X to storage-X
The naming convention spanstore-X will not hold up long, particularly
as we already have multiple storage interfaces, and aggregations are
dependency links aren't spans.

This uses the convention `spanstore-x` as it relates better to what
the components do. It also is closer the the environment variable
`STORAGE_TYPE`. ex `STORAGE_TYPE=cassandra` needs `storage-cassandra`.

Also, in #135 we'll end up with storage component classes, like
`ElasticsearchStorage`, which provide a graph to the various storage
interfaces like `SpanStore`.
77bcd2c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment