-
Notifications
You must be signed in to change notification settings - Fork 332
JIRA: SAMZA-1733 Populating ListGauge metric using DiagnosticsAppender for exceptions #543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 38 commits
5ae1df9
5a3ef7b
75ef5d2
1efed5a
d2922c5
6602fa0
054d507
1f08eae
b598b92
6f19ba0
c22e252
4f08cd1
9f98c57
906afeb
bdc85d4
5c1ed0a
c1884e4
b0695b4
508b6b3
c7821ba
9ba8e5b
64b1cb8
fd77c53
cc4d881
172ab20
863d850
cac4ecb
925473c
2ad0264
60f281c
cd58669
8a66d16
48cbf3e
346affd
7f7885b
92a29b4
e9f4004
28c4fec
edbd200
f8ace28
a4d11b0
6e274e7
7c727f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,6 +56,8 @@ import org.apache.samza.task._ | |
| import org.apache.samza.util.Util | ||
| import org.apache.samza.util._ | ||
| import org.apache.samza.{SamzaContainerStatus, SamzaException} | ||
| import org.apache.samza.diagnostics.DiagnosticsAppender; | ||
| import org.apache.log4j.Logger | ||
|
|
||
| import scala.collection.JavaConverters._ | ||
|
|
||
|
|
@@ -757,6 +759,7 @@ class SamzaContainer( | |
| jmxServer = new JmxServer() | ||
|
|
||
| startMetrics | ||
| startDiagnostics | ||
| startAdmins | ||
| startOffsetManager | ||
| startLocalityManager | ||
|
|
@@ -904,6 +907,17 @@ class SamzaContainer( | |
| }) | ||
| } | ||
|
|
||
| def startDiagnostics { | ||
| if (containerContext.config.getDiagnosticsEnabled) { | ||
| val rootLogger = Logger.getRootLogger | ||
|
|
||
| if (rootLogger.getAppender(classOf[DiagnosticsAppender].getName) == null) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that in log4j2, the API to get appender dynamically also changed: https://logging.apache.org/log4j/2.x/log4j-core/apidocs/org/apache/logging/log4j/core/config/Configuration.html#getAppender(java.lang.String). Please check w/ @PawasChhokra to see what's the best way to make sure the code here is log4j version agnostic.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this condition be (rootLogger.getAppender(classOf[DiagnosticsAppender].getName) != null)? |
||
| info("Starting diagnostics appender.") | ||
| rootLogger.addAppender(new DiagnosticsAppender(this.metrics)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| def startOffsetManager { | ||
| info("Registering task instances with offsets.") | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * 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 org.apache.samza.diagnostics; | ||
|
|
||
| import org.apache.log4j.AppenderSkeleton; | ||
| import org.apache.log4j.spi.LoggingEvent; | ||
| import org.apache.samza.container.SamzaContainerMetrics; | ||
| import org.apache.samza.metrics.ListGauge; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
|
|
||
| /** | ||
| * Provides an in-memory appender that parses LoggingEvents to filter events relevant to diagnostics. | ||
| * Currently, filters exception related events and update an exception metric ({@link ListGauge}) in | ||
| * {@link SamzaContainerMetrics}. | ||
| * | ||
| * When used inconjunction with {@link org.apache.samza.metrics.reporter.MetricsSnapshotReporter} provides a | ||
| * stream of diagnostics-related events. | ||
| */ | ||
| public class DiagnosticsAppender extends AppenderSkeleton { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious. Do we need to have separate implementation for samza jobs using log4j2?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the same implementation can be adopted, as of log4j2.4. |
||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(DiagnosticsAppender.class); | ||
| private final ListGauge<DiagnosticsExceptionEvent> samzaContainerExceptionMetric; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: how is this metric serialized and sent to diagnostic topic? If yes, shouldn't DiagnosticsExceptionEvent be serializable?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Existing |
||
|
|
||
| public DiagnosticsAppender(SamzaContainerMetrics samzaContainerMetrics) { | ||
| this.samzaContainerExceptionMetric = samzaContainerMetrics.exceptions(); | ||
| this.setName(DiagnosticsAppender.class.getName()); | ||
| } | ||
|
|
||
| @Override | ||
| protected void append(LoggingEvent loggingEvent) { | ||
|
|
||
| try { | ||
| // if an event with a non-null throwable is received => exception event | ||
| if (loggingEvent.getThrowableInformation() != null) { | ||
| DiagnosticsExceptionEvent diagnosticsExceptionEvent = | ||
| new DiagnosticsExceptionEvent(loggingEvent.timeStamp, loggingEvent.getThrowableInformation().getThrowable(), | ||
| loggingEvent.getProperties()); | ||
|
|
||
| samzaContainerExceptionMetric.add(diagnosticsExceptionEvent); | ||
| LOG.debug("Received DiagnosticsExceptionEvent " + diagnosticsExceptionEvent); | ||
| } else { | ||
| LOG.debug("Received non-exception event with message " + loggingEvent.getMessage()); | ||
| } | ||
| } catch (Exception e) { | ||
| // blanket catch of all exceptions so as to not impact any job | ||
| LOG.error("Exception in logging event parsing", e); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void close() { | ||
| // Do nothing. | ||
| } | ||
|
|
||
| /** | ||
| * Returns false since this appender requires no layout. | ||
| * @return false | ||
| */ | ||
| @Override | ||
| public boolean requiresLayout() { | ||
| return false; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * 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 org.apache.samza.diagnostics; | ||
|
|
||
| import java.util.Arrays; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
| import java.util.Objects; | ||
|
|
||
|
|
||
| /** | ||
| * This class encapsulates information related to an exception event that is useful for diagnostics. | ||
| * It used to define container, task, and other metrics as | ||
| * {@link org.apache.samza.metrics.ListGauge} of type {@link DiagnosticsExceptionEvent}. | ||
| */ | ||
| public class DiagnosticsExceptionEvent { | ||
|
|
||
| private long timestamp; // the timestamp associated with this exception | ||
| private Class exceptionType; // store the exception type separately | ||
| private Throwable throwable; | ||
| private Map mdcMap; | ||
| // the MDC map associated with this exception, used to store/obtain any context associated with the throwable | ||
|
|
||
| public DiagnosticsExceptionEvent() { | ||
| } | ||
|
|
||
| public DiagnosticsExceptionEvent(long timestampMillis, Throwable throwable, Map mdcMap) { | ||
| this.throwable = throwable; | ||
| this.exceptionType = throwable.getClass(); | ||
| this.timestamp = timestampMillis; | ||
| this.mdcMap = new HashMap(mdcMap); | ||
| } | ||
|
|
||
| public long getTimestamp() { | ||
| return timestamp; | ||
| } | ||
|
|
||
| public Throwable getThrowable() { | ||
| return this.throwable; | ||
| } | ||
|
|
||
| public Class getExceptionType() { | ||
| return this.exceptionType; | ||
| } | ||
|
|
||
| public Map getMdcMap() { | ||
| return mdcMap; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (o == null || getClass() != o.getClass()) { | ||
| return false; | ||
| } | ||
| DiagnosticsExceptionEvent that = (DiagnosticsExceptionEvent) o; | ||
|
|
||
| // Throwable provides no equals impl, so we assume message & stacktrace equality suffices | ||
| return timestamp == that.timestamp && this.exceptionType.equals(that.exceptionType) && mdcMap.equals(that.mdcMap) | ||
| && this.throwable.getMessage().equals(that.throwable.getMessage()) && Arrays.equals( | ||
| this.throwable.getStackTrace(), that.throwable.getStackTrace()); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(timestamp, exceptionType, throwable, mdcMap); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,9 +19,11 @@ | |
|
|
||
| package org.apache.samza.metrics.reporter | ||
|
|
||
| import java.util | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to be redundant, if we remove the prefix to HashMap in line 59. |
||
| import java.util.Collections | ||
| import java.util.HashMap | ||
| import java.util.Map | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: whitespace change should be avoided. |
||
| import scala.collection.JavaConverters._ | ||
|
|
||
| object Metrics { | ||
|
|
@@ -52,4 +54,8 @@ class Metrics(metrics: Map[String, Map[String, Object]]) { | |
| def get(group: String) = immutableMetrics.get(group) | ||
|
|
||
| def getAsMap(): Map[String, Map[String, Object]] = Collections.unmodifiableMap(immutableMetrics) | ||
|
|
||
| def this() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did a code search and couldn't find any reference to this default constructor. What's the reason to add it? |
||
| this(new util.HashMap[String, Map[String, Object]]()) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * 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 org.apache.samza.serializers; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
| import org.apache.samza.metrics.reporter.MetricsSnapshot; | ||
| import org.codehaus.jackson.map.ObjectMapper; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
|
|
||
| public class MetricsSnapshotSerdeV2 implements Serde<MetricsSnapshot> { | ||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(MetricsSnapshotSerdeV2.class); | ||
| private final ObjectMapper objectMapper; | ||
|
|
||
| public MetricsSnapshotSerdeV2() { | ||
| objectMapper = new ObjectMapper(); | ||
| objectMapper.enableDefaultTyping(ObjectMapper.DefaultTyping.OBJECT_AND_NON_CONCRETE); | ||
| } | ||
|
|
||
| @Override | ||
| public MetricsSnapshot fromBytes(byte[] bytes) { | ||
| try { | ||
| return MetricsSnapshot.fromMap( | ||
| objectMapper.readValue(bytes, new HashMap<String, Map<String, Object>>().getClass())); | ||
| } catch (IOException e) { | ||
| LOG.info("Exception while deserializing", e); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public byte[] toBytes(MetricsSnapshot metricsSnapshot) { | ||
| try { | ||
| return objectMapper.writeValueAsString(convertMap(metricsSnapshot.getAsMap())).getBytes("UTF-8"); | ||
| } catch (IOException e) { | ||
| LOG.info("Exception while serializing", e); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| /** Metrics returns an UnmodifiableMap. | ||
| * Unmodifiable maps should not be serialized with type, because UnmodifiableMap cannot be deserialized. | ||
| * So we convert to HashMap. | ||
| */ | ||
| private HashMap convertMap(Map<String, Object> map) { | ||
| HashMap retVal = new HashMap(map); | ||
| for (Map.Entry<String, Object> entry : map.entrySet()) { | ||
| if (entry.getValue() instanceof Map) { | ||
| retVal.put(entry.getKey(), convertMap((Map<String, Object>) entry.getValue())); | ||
| } | ||
| } | ||
| return retVal; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * 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 org.apache.samza.serializers; | ||
|
|
||
| import org.apache.samza.config.Config; | ||
| import org.apache.samza.metrics.reporter.MetricsSnapshot; | ||
|
|
||
|
|
||
| public class MetricsSnapshotSerdeV2Factory implements SerdeFactory<MetricsSnapshot> { | ||
| @Override | ||
| public Serde<MetricsSnapshot> getSerde(String name, Config config) { | ||
| return new MetricsSnapshotSerdeV2(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not using log4j directly in samza-core. We always use slf4j libraries in all our source code, except for the samza-log4j module.