You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@phoenix.apache.org by elilevine <gi...@git.apache.org> on 2014/09/16 02:20:55 UTC
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
GitHub user elilevine opened a pull request:
https://github.com/apache/phoenix/pull/15
Logging custom annotations for PHOENIX-1198
This PR adds custom annotations to log lines generated in Phoenix's client-side code. It builds on PR https://github.com/apache/phoenix/pull/13 and uses the same mechanism to pass custom annotations to connections. These custom annotations are only added to client-side code logging that is within the context of a user operation (meaning a PhoenixConnection is available). No new log lines are added here. Only existing log lines are changed.
Usage:
1. Add custom annotation properties when creating a Phoenix Connection. e.g. `phoenix.trace.custom.annotation.a1=v1`
2. Log lines in (some of) Phoenix's client-side code would look like this: `{a1=v1} something logged`
Ignore the following files here, since they are covered in PR #13: BaseTracingTestIT.java, PhoenixTracingEndToEndIT.java, TraceReader.java, Tracing.java, JDBCUtilTest.java
@jtaylor-sfdc
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/elilevine/apache-phoenix logging-custom-annotations
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/15.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #15
----
commit 786c6e6318c0945f818f1498545efeba43fb4b05
Author: Eli Levine <el...@apache.org>
Date: 2014-09-11T19:17:54Z
First pass at custom tracing annotations
commit 07247764ed93f293fd8e99ba4ba36e32cdf99068
Author: Eli Levine <el...@apache.org>
Date: 2014-09-11T19:28:03Z
Merge upstream changes
commit 7a0f552589653a5d9e2f60a287e867e450e3861e
Author: Eli Levine <el...@apache.org>
Date: 2014-09-15T23:06:20Z
Custom annotations in logs for client-side code that works within the context of a user operation (PhoenixConnection is available)
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by jyates <gi...@git.apache.org>.
Github user jyates commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/15#discussion_r17636539
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/trace/util/Tracing.java ---
@@ -257,6 +264,18 @@ public static Span child(Span s, String d) {
public static CallWrapper withTracing(PhoenixConnection conn, String desc) {
return new TracingWrapper(conn, desc);
}
+
+ private static void addCustomAnnotationsToSpan(@Nullable Span span, @NotNull PhoenixConnection conn) {
+ Preconditions.checkNotNull(conn);
+
+ if (span != null) {
--- End diff --
nit: flip the logic and save the indent for readabililty
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by jyates <gi...@git.apache.org>.
Github user jyates commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/15#discussion_r17636321
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/trace/BaseTracingTestIT.java ---
@@ -86,9 +89,16 @@ public static Connection getConnectionWithoutTracing(Properties props) throws SQ
conn.setAutoCommit(false);
return conn;
}
+
+ public static Connection getTracingConnection() throws Exception {
+ return getTracingConnection(new HashMap<String, String>(0));
--- End diff --
could use Collections.emptyMap() here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on the pull request:
https://github.com/apache/phoenix/pull/15#issuecomment-55979859
Had an offline conversation with Jesse and he had a good idea. Can we change the property name by dropping the "trace" part of it and have this instead? I don't think having "custom" in there adds anything either. How about just phoenix.annotation? Then you can use these for tracing and logging both.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/15#discussion_r17644459
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/util/LogUtil.java ---
@@ -0,0 +1,34 @@
+/*
+ * 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.phoenix.util;
+
+import java.util.Map;
+
+public class LogUtil {
--- End diff --
Tracing-only changes are in PR #13. This PR was supposed to be only for logging-related code but I screwed it up because I pulled in my tracing changes used in #13 because for logging I am using the same way to pass annotations in. Tried to make that clear in this PR's description but clearly failed. Sorry about that. Maybe take a look at #13 that has tracing-only changes?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/15#discussion_r17644317
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/trace/BaseTracingTestIT.java ---
@@ -86,9 +89,16 @@ public static Connection getConnectionWithoutTracing(Properties props) throws SQ
conn.setAutoCommit(false);
return conn;
}
+
+ public static Connection getTracingConnection() throws Exception {
+ return getTracingConnection(new HashMap<String, String>(0));
--- End diff --
Generics gets screwed up if emptyMap() is used in the parameter. I'll see what I can do to make this cleaner.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by jyates <gi...@git.apache.org>.
Github user jyates commented on the pull request:
https://github.com/apache/phoenix/pull/15#issuecomment-55827917
This would be a lot nicer if you didn't mix the logging changes with the tracing changes - the changelist would be much more succinct and to the point. Otherwise, seems like the right approach, though it could use some cleanup (docs, readability, etc)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/15#discussion_r17579564
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java ---
@@ -214,11 +218,16 @@ public boolean prune(PTable table) {
// setup tracing, if its enabled
this.sampler = Tracing.getConfiguredSampler(this);
+ this.customTracingAnnotations = ImmutableMap.copyOf(JDBCUtil.getCustomTracingAnnotations(url, info));
--- End diff --
Good call. Will add.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by jyates <gi...@git.apache.org>.
Github user jyates commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/15#discussion_r17636458
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/util/JDBCUtil.java ---
@@ -63,6 +68,39 @@ public static String findProperty(String url, Properties info, String propName)
}
return propValue;
}
+
+ private static Map<String, String> getCombinedConnectionProperties(String url, Properties info) {
--- End diff --
nit: docs
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/15#discussion_r17644266
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/trace/util/Tracing.java ---
@@ -43,6 +47,8 @@
import org.cloudera.htrace.wrappers.TraceRunnable;
import com.google.common.base.Function;
+import com.google.common.base.Preconditions;
+import com.sun.istack.NotNull;
--- End diff --
Contracts are worthless if they are not enforceable. ;o)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by ddispaltro <gi...@git.apache.org>.
Github user ddispaltro commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/15#discussion_r17644319
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/trace/util/Tracing.java ---
@@ -257,6 +264,18 @@ public static Span child(Span s, String d) {
public static CallWrapper withTracing(PhoenixConnection conn, String desc) {
return new TracingWrapper(conn, desc);
}
+
+ private static void addCustomAnnotationsToSpan(@Nullable Span span, @NotNull PhoenixConnection conn) {
+ Preconditions.checkNotNull(conn);
+
+ if (span != null) {
--- End diff --
```java
if (span == null)
return;
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by jyates <gi...@git.apache.org>.
Github user jyates commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/15#discussion_r17636489
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/trace/util/Tracing.java ---
@@ -43,6 +47,8 @@
import org.cloudera.htrace.wrappers.TraceRunnable;
import com.google.common.base.Function;
+import com.google.common.base.Preconditions;
+import com.sun.istack.NotNull;
--- End diff --
i think if you mark something as not null, you don't need to do the null check - kind of the deal with programming by contract, ya?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/15#discussion_r17578973
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java ---
@@ -214,11 +218,16 @@ public boolean prune(PTable table) {
// setup tracing, if its enabled
this.sampler = Tracing.getConfiguredSampler(this);
+ this.customTracingAnnotations = ImmutableMap.copyOf(JDBCUtil.getCustomTracingAnnotations(url, info));
--- End diff --
How about adding a custom annotation for known properties like TENANT_ID and CURRENT_SCN?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by jyates <gi...@git.apache.org>.
Github user jyates commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/15#discussion_r17636519
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/trace/util/Tracing.java ---
@@ -153,6 +159,7 @@ public static void setSampling(Properties props, Frequency freq) {
public static TraceScope startNewSpan(PhoenixConnection connection, String string) {
Sampler<?> sampler = connection.getSampler();
TraceScope scope = Trace.startSpan(string, sampler);
+ addCustomAnnotationsToSpan(scope.getSpan(), connection);
--- End diff --
customAnnotations seems the wrong name. Maybe something like "addConnectionAnnotations"?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on the pull request:
https://github.com/apache/phoenix/pull/15#issuecomment-56899160
Merged now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on the pull request:
https://github.com/apache/phoenix/pull/15#issuecomment-55981263
I like it. Good call.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by jyates <gi...@git.apache.org>.
Github user jyates commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/15#discussion_r17636409
--- Diff: phoenix-hadoop-compat/src/main/java/org/apache/phoenix/trace/TracingCompat.java ---
@@ -86,4 +86,4 @@ public static final String getTraceMetricName(long traceId) {
public static final String getTraceMetricName(String traceId) {
return METRIC_SOURCE_KEY + traceId;
}
-}
\ No newline at end of file
+}
--- End diff --
nit: unneccessary change
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/15#discussion_r17644279
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/trace/util/Tracing.java ---
@@ -257,6 +264,18 @@ public static Span child(Span s, String d) {
public static CallWrapper withTracing(PhoenixConnection conn, String desc) {
return new TracingWrapper(conn, desc);
}
+
+ private static void addCustomAnnotationsToSpan(@Nullable Span span, @NotNull PhoenixConnection conn) {
+ Preconditions.checkNotNull(conn);
+
+ if (span != null) {
--- End diff --
don't follow
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine closed the pull request at:
https://github.com/apache/phoenix/pull/15
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Logging custom annotations for PHOENIX-1198
Posted by jyates <gi...@git.apache.org>.
Github user jyates commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/15#discussion_r17636396
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/util/LogUtil.java ---
@@ -0,0 +1,34 @@
+/*
+ * 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.phoenix.util;
+
+import java.util.Map;
+
+public class LogUtil {
--- End diff --
This seems a strange way to handle it, when used in practice. Its not entirely clear that all the log elements should be annotated. In fact, it would be nice, at least in this initial pass, to separate the logging from the tracing in terms of the per-connection annotations.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---