Remove Guava dependencies from SDK #2148

anuraaga · 2020-11-27T08:18:28Z

For #1944

Remaining are gRPC-based artifacts (always use guava), opencensus shim (it's going away in new implementation type), and jfr (will probably use weak-lock-free for it after raphw/weak-lock-free#12)

Oberon00 · 2020-11-27T08:29:43Z

CC @thisthat

Oberon00

Thank you for the work! Since this is a rather big PR, maybe it could be split up a bit. zPages may be worth splitting out (if we want to get rid of Guava there at all), and the core SDK (sdk-all + dependencies + maybe core propagators/exporters) could be split out since that part would be straightforward.

Oberon00 · 2020-11-27T08:43:56Z

sdk-extensions/aws/src/main/java/io/opentelemetry/sdk/extension/aws/resource/DockerHelper.java

  }

-  @VisibleForTesting
+  // Visible for testing


I think we should add such an annotation to the API package or some util package. It is useful documentation and a bit nicer than a comment.

The annotation has been around for a looong time and I've never seen any tooling use it. Is it much better than a comment? Then we wouldn't need to maintain the annotation.

Hmm, no strong opinion. If you think the annotation fits badly, comments should be fine too.

Oberon00 · 2020-11-27T08:45:27Z

sdk-extensions/aws/src/main/java/io/opentelemetry/sdk/extension/aws/resource/Ec2Resource.java

+    }
+    int b;
+    while ((b = is.read()) != -1) {
+      os.write(b);


Byte-wise reading looks quite inefficient.

Oops for some reason thought this was in-memory

Wouldn't the overhead be even worse then, relatively speaking? EDIT: Depending on whether the I/O does some internal buffering.

Oberon00 · 2020-11-27T08:46:39Z

sdk-extensions/aws/src/main/java/io/opentelemetry/sdk/extension/aws/resource/EcsResource.java

  private boolean isOnEcs() {
-    return (!Strings.isNullOrEmpty(sysEnv.get(ECS_METADATA_KEY_V3))
-        || !Strings.isNullOrEmpty(sysEnv.get(ECS_METADATA_KEY_V4)));
+    String metadata = sysEnv.getOrDefault(ECS_METADATA_KEY_V3, "");


With the getOrDefault, can't you keep it as a single expression? Also, I think we have some isNullOrEmpty helper already in the propagator-utils.

Yeah don't think we can use propagator classes here. We could move it to API to the internal package, but so far we don't leak internal classes across artifacts in this repo. I think it's ok though so if others thing so too I could do that.

Oberon00 · 2020-11-27T08:47:49Z

sdk-extensions/aws/src/main/java/io/opentelemetry/sdk/extension/aws/resource/EksResource.java

+    try (BufferedInputStream is = new BufferedInputStream(new FileInputStream(file))) {
+      while (true) {
+        int read = is.read(result);
+        if (read < 0) {


Shouldn't one abort also if read == 0? AFAIK this is documented to only happen at EOF.

Oberon00 · 2020-11-27T08:50:45Z

sdk-extensions/aws/src/main/java/io/opentelemetry/sdk/extension/aws/resource/EksResource.java

    return "";
  }
+
+  private static byte[] readAllBytes(String path) throws IOException {


Even in Java 7, there is a stdlib API that does that: https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#readAllBytes(java.nio.file.Path)

Not on Android - but I realized, this module should not have the Android sniffer on... Nice!

Oberon00 · 2020-11-27T08:57:41Z

sdk-extensions/zpages/src/main/java/io/opentelemetry/sdk/extension/zpages/ZPageHttpHandler.java

+                      .filter(s -> !s.isEmpty())
+                      .map(String::trim)


Suggested change

.filter(s -> !s.isEmpty())

.map(String::trim)

Should not be needed. Also spaces may be relevant here.

Oberon00 · 2020-11-27T09:06:03Z

sdk-extensions/zpages/src/main/java/io/opentelemetry/sdk/extension/zpages/ZPageHttpHandler.java

+                if (keyValuePair.get(0).equals(PARAM_SPAN_NAME)) {
+                  try {
+                    queryMap.put(
+                        keyValuePair.get(0), URLDecoder.decode(keyValuePair.get(1), "UTF-8"));


Why only decode selectively here? Shouldn't all keys and values be decoded?

Think I agree - would probably fix that separately from guava refactoring.

Oberon00 · 2020-11-27T09:07:13Z

sdk/common/src/main/java/io/opentelemetry/sdk/common/export/ConfigBuilder.java

  public T readProperties(Properties properties) {
-    return fromConfigMap(Maps.fromProperties(properties), NamingConvention.DOT);
+    Map<String, String> map = new HashMap<>(properties.size());
+    properties.forEach((key, value) -> map.put((String) key, (String) value));


I think a normal foreach loop would be more readable.

Hmm - I've always appreciated Java 8's Map.forEach since Entry can be a bit gross :)

Alternatively, how about some casting? https://stackoverflow.com/a/17209434/2128694

Map<String, String> map = (Map<String, String>)(Map)properties;

Oberon00 · 2020-11-27T09:09:06Z

sdk/common/src/main/java/io/opentelemetry/sdk/resources/ResourcesConfig.java

+            Collections.unmodifiableSet(
+                Arrays.stream(stringValue.split(","))
+                    .filter(s -> !s.isEmpty())
+                    .map(String::trim)


I think you might need to swap the order of trim/isEmpty to match Guava. AFAIK omitEmptyStrings only sets a boolean. But I may be wrong here.

Whether or not it matches guava, this logic seems right. Values that are only whitespace shouldn't be used, I don't think.

My not splitting the PRs up in advance caused this review to be a bit hectic, sorry about that. While GitHub doesn't have the outdated mark here, if I remember correctly, the code used to be in the wrong order and @Oberon00 commented and I fixed it so it should be good now.

Oberon00 · 2020-11-27T09:09:38Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/config/TraceConfig.java

-          traceConfig.getMaxNumberOfAttributesPerLink() > 0, "maxNumberOfAttributesPerLink");
-      Preconditions.checkArgument(
-          traceConfig.getMaxLengthOfAttributeValues() >= -1, "maxLengthOfAttributeValues");
+      if (traceConfig.getMaxNumberOfAttributes() <= 0) {


Didn't we have some Utils.checkArgument function?

Yeah we have it in API - if we're ok with cross-referencing from other artifacts (guess it's ok) I'll use it

Yeah, the SDK uses API classes quite a bit (obviously because it has to implement it!). I think this is a fine usage, as long as it's not in a strictly "internal" package.

anuraaga

Sure let me split zpages out

anuraaga · 2020-11-27T09:49:43Z

sdk-extensions/aws/src/main/java/io/opentelemetry/sdk/extension/aws/resource/DockerHelper.java

  }

-  @VisibleForTesting
+  // Visible for testing


The annotation has been around for a looong time and I've never seen any tooling use it. Is it much better than a comment? Then we wouldn't need to maintain the annotation.

anuraaga · 2020-11-27T09:51:16Z

sdk-extensions/aws/src/main/java/io/opentelemetry/sdk/extension/aws/resource/EcsResource.java

  private boolean isOnEcs() {
-    return (!Strings.isNullOrEmpty(sysEnv.get(ECS_METADATA_KEY_V3))
-        || !Strings.isNullOrEmpty(sysEnv.get(ECS_METADATA_KEY_V4)));
+    String metadata = sysEnv.getOrDefault(ECS_METADATA_KEY_V3, "");


Yeah don't think we can use propagator classes here. We could move it to API to the internal package, but so far we don't leak internal classes across artifacts in this repo. I think it's ok though so if others thing so too I could do that.

anuraaga · 2020-11-27T09:53:01Z

sdk-extensions/aws/src/main/java/io/opentelemetry/sdk/extension/aws/resource/Ec2Resource.java

+    }
+    int b;
+    while ((b = is.read()) != -1) {
+      os.write(b);


Oops for some reason thought this was in-memory

anuraaga · 2020-11-27T09:57:35Z

sdk-extensions/zpages/src/main/java/io/opentelemetry/sdk/extension/zpages/ZPageHttpHandler.java

-    return ImmutableMap.copyOf(queryMap);
+    Arrays.stream(queryString.split("&"))
+        .filter(s -> !s.isEmpty())
+        .map(String::trim)


Hmm - not sure just reproing the current code for now

anuraaga · 2020-11-27T09:58:09Z

sdk-extensions/zpages/src/main/java/io/opentelemetry/sdk/extension/zpages/ZPageHttpHandler.java

+                if (keyValuePair.get(0).equals(PARAM_SPAN_NAME)) {
+                  try {
+                    queryMap.put(
+                        keyValuePair.get(0), URLDecoder.decode(keyValuePair.get(1), "UTF-8"));


Think I agree - would probably fix that separately from guava refactoring.

anuraaga · 2020-11-27T09:58:48Z

sdk/common/src/main/java/io/opentelemetry/sdk/common/export/ConfigBuilder.java

  public T readProperties(Properties properties) {
-    return fromConfigMap(Maps.fromProperties(properties), NamingConvention.DOT);
+    Map<String, String> map = new HashMap<>(properties.size());
+    properties.forEach((key, value) -> map.put((String) key, (String) value));


Hmm - I've always appreciated Java 8's Map.forEach since Entry can be a bit gross :)

anuraaga · 2020-11-27T09:59:32Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/config/TraceConfig.java

-          traceConfig.getMaxNumberOfAttributesPerLink() > 0, "maxNumberOfAttributesPerLink");
-      Preconditions.checkArgument(
-          traceConfig.getMaxLengthOfAttributeValues() >= -1, "maxLengthOfAttributeValues");
+      if (traceConfig.getMaxNumberOfAttributes() <= 0) {


Yeah we have it in API - if we're ok with cross-referencing from other artifacts (guess it's ok) I'll use it

anuraaga · 2020-11-27T10:00:33Z

...xtensions/zpages/src/main/java/io/opentelemetry/sdk/extension/zpages/TracezZPageHandler.java

-    @Override
-    public int compare(Event e1, Event e2) {
-      return Long.compare(e1.getEpochNanos(), e2.getEpochNanos());
+  private static String escapeHtml(String html) {


Yeah this is the one line that gets most benefit from Guava. I'm mixed, 2MB dependency just for this function, mostly :)

Oberon00 · 2020-11-27T10:01:15Z

sdk-extensions/aws/src/main/java/io/opentelemetry/sdk/extension/aws/resource/Ec2Resource.java

-      os.write(b);
+    byte[] buf = new byte[8192];
+    while (is.read(buf) != -1) {
+      os.write(buf);


You can't write the full buffer here. You need to check how many bytes you actually read into the buffer, which may be less.

Oberon00

Some cleanups should be considered, but all in all 👍

sdk/all/src/main/java/io/opentelemetry/sdk/OpenTelemetrySdk.java

Oberon00 · 2020-11-27T10:20:02Z

sdk/common/src/main/java/io/opentelemetry/sdk/common/export/ConfigBuilder.java

  public T readProperties(Properties properties) {
-    return fromConfigMap(Maps.fromProperties(properties), NamingConvention.DOT);
+    Map<String, String> map = new HashMap<>(properties.size());
+    properties.forEach((key, value) -> map.put((String) key, (String) value));


Alternatively, how about some casting? https://stackoverflow.com/a/17209434/2128694

Map<String, String> map = (Map<String, String>)(Map)properties;

jkwatson

Thanks!

anuraaga requested review from Oberon00, arminru, bogdandrutu, carlosalberto, jkwatson, pavolloffay, thisthat and tylerbenson as code owners November 27, 2020 08:18

Oberon00 reviewed Nov 27, 2020

View reviewed changes

anuraaga commented Nov 27, 2020

View reviewed changes

Oberon00 reviewed Nov 27, 2020

View reviewed changes

Remove Guava from SDK

d962c1c

anuraaga force-pushed the remove-guava branch from 2106b56 to d962c1c Compare November 27, 2020 10:02

anuraaga changed the title ~~Remove Guava dependencies from src/main in most places.~~ Remove Guava dependencies from SDK Nov 27, 2020

Oberon00 approved these changes Nov 27, 2020

View reviewed changes

Clean

d8476a2

jkwatson approved these changes Dec 1, 2020

View reviewed changes

jkwatson merged commit 18355e9 into open-telemetry:master Dec 1, 2020

beanliu mentioned this pull request Jan 28, 2021

Histogram Aggregation - core SDK implementation #2581

Closed

Remove Guava dependencies from SDK #2148

Remove Guava dependencies from SDK #2148

Uh oh!

Conversation

anuraaga commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Oberon00 commented Nov 27, 2020

Uh oh!

Oberon00 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oberon00 Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuraaga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

anuraaga commented Nov 27, 2020 •

edited

Loading

Oberon00 Nov 27, 2020 •

edited

Loading