fix and/or ignore spotbugs errors #1325

imavroukakis · 2020-10-05T09:55:45Z

There are a couple of spotbugs complaints that I'd like some feedback on

H C NP: Null passed for non-null parameter of io.opentelemetry.trace.Span.setAttribute(AttributeKey, Object) in io.opentelemetry.instrumentation.auto.grpc.common.GrpcHelper.prepareSpan(Span, String, InetSocketAddress, boolean)  Method invoked at GrpcHelper.java:[line 62]

API for setAttribute strongly advises against null. If it's still ok to pass null here we can add a spotbugs exclusion otherwise, is an empty string an acceptable value?

H B Nm: The class name io.opentelemetry.javaagent.tooling.config.ConfigBuilder shadows the simple name of the superclass io.opentelemetry.sdk.common.export.ConfigBuilder  At ConfigBuilder.java:[lines 26-61]

I think this can be safely ignored

H D UC: Method io.opentelemetry.instrumentation.api.tracer.utils.NetPeerUtils.setNetPeer(Span, String, String, int) seems to be useless  At NetPeerUtils.java:[line 86]

This is probably a false positive as it is used in HttpClientTracer, do you agree?

gradle/spotbugs-exclude.xml

iNikem · 2020-10-05T11:05:25Z

smoke-tests/src/test/groovy/io/opentelemetry/smoketest/SmokeTest.groovy

@@ -46,7 +46,7 @@ abstract class SmokeTest extends Specification {

  private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper()

-  protected static OkHttpClient client = OkHttpUtils.client()
+  protected static final OkHttpClient CLIENT = OkHttpUtils.client()


Can we instead ignore this rule? I think @trask will object, but why static final variable MUST be in uppercase?

spotBugs actually doesn't complain about the uppercase, just the missing final declaration. As for the uppercase, I believe it's a JLS convention for constants, but given the naming exception done for loggers, I don't see why it cannot be the same here - just possibly, have to look out for other static analysis checkers complaining about it ;)

I don't think JLS has any naming conventions. Has it?

https://docs.oracle.com/javase/specs/jls/se11/html/jls-6.html look for "Constant Names" section.
Emphasis, mine.

The names of constants in interface types should be, and final variables of class types may conventionally be, a sequence of one or more words, acronyms, or abbreviations, all uppercase

The capitalisation could have been a misrepresentation though, and the intent may have been to use them for primitives, but it somehow stuck for everything

Can we instead ignore this rule? I think @trask will object, but why static final variable MUST be in uppercase?

I do not object 😂

static final variables should only be uppercase if they are constants, see https://google.github.io/styleguide/javaguide.html#s5.2.4-constant-names for "what is a constant"

iNikem · 2020-10-05T11:07:54Z

There are a couple of spotbugs complaints that I'd like some feedback on

H C NP: Null passed for non-null parameter of io.opentelemetry.trace.Span.setAttribute(AttributeKey, Object) in io.opentelemetry.instrumentation.auto.grpc.common.GrpcHelper.prepareSpan(Span, String, InetSocketAddress, boolean)  Method invoked at GrpcHelper.java:[line 62]

Should be fixed: set attribute only if it is not null.

H B Nm: The class name io.opentelemetry.javaagent.tooling.config.ConfigBuilder shadows the simple name of the superclass io.opentelemetry.sdk.common.export.ConfigBuilder  At ConfigBuilder.java:[lines 26-61]

@mateuszrzeszutek please remind me, why do we need this class which largely duplicates superclass?

H D UC: Method io.opentelemetry.instrumentation.api.tracer.utils.NetPeerUtils.setNetPeer(Span, String, String, int) seems to be useless  At NetPeerUtils.java:[line 86]

Main branch does not 86 lines in NetPeerUtils :) Can you rebase your branch and try again?

mateuszrzeszutek · 2020-10-05T11:36:34Z

H B Nm: The class name io.opentelemetry.javaagent.tooling.config.ConfigBuilder shadows the simple name of the superclass io.opentelemetry.sdk.common.export.ConfigBuilder At ConfigBuilder.java:[lines 26-61]

@mateuszrzeszutek please remind me, why do we need this class which largely duplicates superclass?

It doesn't really duplicate its superclass, it only extends those methods that it absolutely needs to. Though I agree that its name is somewhere between "uncreative" and "terrible", but I didn't have any better idea when I wrote it 😛

imavroukakis · 2020-10-05T11:43:07Z

export.ConfigBuilder

how about ExportConfigBuilder ?

* GrpcHelper: methodName should not be set in the span if null * GrpcHelper: fix warnings instead of hiding by using Callable

imavroukakis · 2020-10-05T11:54:45Z

Main branch does not 86 lines in NetPeerUtils :) Can you rebase your branch and try again?
Now it complains about line 75 😆

H D UC: Method io.opentelemetry.instrumentation.api.tracer.utils.NetPeerUtils.setNetPeer(Span, String, String, int) seems to be useless  At NetPeerUtils.java:[line 75]

mateuszrzeszutek · 2020-10-05T12:01:50Z

how about ExportConfigBuilder ?

I believe that io.opentelemetry.sdk.common.export.ConfigBuilder is the full name of the super class in the OTel SDK. Hmm, how about AgentConfigBuilder?

imavroukakis · 2020-10-05T12:44:33Z

Odd failures for :instrumentation:servlet:glassfish-testing:test and :instrumentation:jms-1.1:test I can't replicate them locally.

iNikem · 2020-10-05T12:57:07Z

jms is a known flaky test

imavroukakis · 2020-10-05T13:03:07Z

jms is a known flaky test

ah thanks, @iNikem , is there any way to manually kick the build again?

iNikem · 2020-10-05T13:29:39Z

I will wait, maybe #1327 will fix this

anuraaga · 2020-10-05T15:00:15Z

CircleCI is flaky lately (maybe running out of money with all the free alternatives? :P) Sorry if it looks a little weird @imavroukakis but we may close/open PRs to retrigger the build :)

...gent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/config/AgentConfigBuilder.java

imavroukakis · 2020-10-06T08:58:29Z

Finally, I know how it happens but not why

This works

new Callable<Object>() {
   public Object call() throws Exception {
      resp.sendRedirect(endpoint.getBody());
         return null;
      }
});

This doesn't:

() -> {
  resp.sendRedirect(endpoint.getBody());
  return null;
})

I don't quite know why the test chokes with the lambda, maybe this has to do something with how it's called from Groovy?

mateuszrzeszutek · 2020-10-06T09:09:44Z

I don't quite know why the test chokes with the lambda, maybe this has to do something with how it's called from Groovy?

That might be caused by the fact that Callables are instrumented by the java-concurrent instrumentation and Groovy Closures are not.

imavroukakis · 2020-10-06T09:27:53Z

I don't quite know why the test chokes with the lambda, maybe this has to do something with how it's called from Groovy?

That might be caused by the fact that Callables are instrumented by the java-concurrent instrumentation and Groovy Closures are not.

Hmm, I don't understand your point, can you explain? The issue here is that, whilst the Groovy Closure and Java Callable work fine, when the Callable is expressed as a lambda, it's as if the Glassfish endpoint is not registered (this is a guess as I'm seeing 404s)

mateuszrzeszutek · 2020-10-06T09:53:47Z

whilst the Groovy Closure and Java Callable work fine

Wait, Groovy Closure works fine? Then please disregard my previous theory 😄
(Callable is instrumented to carry the context/scope with a current span that started it; I thought that groovy Closures were losing this piece of data and that was causing those tests to fail)

iNikem · 2020-10-06T10:24:20Z

Beats me why CircleCI does not kick in...

imavroukakis · 2020-10-06T10:33:19Z

whilst the Groovy Closure and Java Callable work fine

Wait, Groovy Closure works fine? Then please disregard my previous theory 😄
(Callable is instrumented to carry the context/scope with a current span that started it; I thought that groovy Closures were losing this piece of data and that was causing those tests to fail)

😄 it's just the Callable when expressed as a lambda that chokes

iNikem · 2020-10-06T10:39:55Z

@imavroukakis I am still looking into why it fails to kick in, but please run ./gradlew codenarcTest task and fix failures.

imavroukakis · 2020-10-06T11:13:02Z

./gradlew codenarcTest

Guess we have to go full uppercase for this @iNikem after all.

iNikem · 2020-10-06T11:39:13Z

@imavroukakis I don't know if this is important, but you may want to fix this:

imavroukakis · 2020-10-06T12:01:28Z

@imavroukakis I don't know if this is important, but you may want to fix this:

Ah thanks, it seems to be picking up the wrong key in GH.

imavroukakis · 2020-10-06T12:38:43Z

circleCI exploded

* Exception is:
java.lang.OutOfMemoryError: Java heap space
	at org.gradle.api.internal.file.archive.impl.FileZipInput.<init>(FileZipInput.java:71)
	at org.gradle.api.internal.file.archive.impl.FileZipInput.create(FileZipInput.java:43)
	at org.gradle.api.internal.changedetection.state.Zip

imavroukakis · 2020-10-06T14:24:13Z

And now it's run out of space.apparently.

> Task :smoke-tests:test
java.io.IOException: No space left on device
com.esotericsoftware.kryo.KryoException: java.io.IOException: No space left on device
	at com.esotericsoftware.kryo.io.Output.flush(Output.java:165)
	at com.esotericsoftware.kryo.io.Output.require(Output.java:142)

iNikem · 2020-10-06T15:34:10Z

Yea, I am trying to understand that disk space issue

trask

Thanks @imavroukakis 👍

I'll submit a separate PR to update the codenarc configuration to avoid having to use uppercase for (non-constant) static final fields (in groovy)

imavroukakis requested review from anuraaga, iNikem, jkwatson, mateuszrzeszutek, trask and tylerbenson as code owners October 5, 2020 09:55

imavroukakis mentioned this pull request Oct 5, 2020

spotBugs task complains about io.opentelemetry.instrumentation.auto.api.concurrent.State #1319

Closed

iNikem reviewed Oct 5, 2020

View reviewed changes

mateuszrzeszutek approved these changes Oct 5, 2020

View reviewed changes

imavroukakis added 3 commits October 5, 2020 12:50

fix and/or ignore spotbugs errors

32e229a

fix GrpcHelper and TestServlets warning

a346dd0

* GrpcHelper: methodName should not be set in the span if null * GrpcHelper: fix warnings instead of hiding by using Callable

lowercase client

bd9cf5d

imavroukakis force-pushed the fix-spotbugs-violations branch from 003b520 to bd9cf5d Compare October 5, 2020 12:00

anuraaga closed this Oct 5, 2020

anuraaga reopened this Oct 5, 2020

imavroukakis added 2 commits October 5, 2020 16:03

renames agent ConfigBuilder

63f6dcd

fix incorrect patterns

3b439de

imavroukakis closed this Oct 5, 2020

imavroukakis reopened this Oct 5, 2020

mateuszrzeszutek reviewed Oct 5, 2020

View reviewed changes

...gent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/config/AgentConfigBuilder.java Outdated Show resolved Hide resolved

restore Groovy closures to fix build

5324166

replace groovy closure with Callable

8c3bcf6

imavroukakis closed this Oct 6, 2020

imavroukakis reopened this Oct 6, 2020

imavroukakis closed this Oct 6, 2020

imavroukakis reopened this Oct 6, 2020

codenarc insists on uppercase for psf

15425ad

iNikem closed this Oct 6, 2020

iNikem reopened this Oct 6, 2020

spotless fixes

c23d919

imavroukakis closed this Oct 6, 2020

imavroukakis reopened this Oct 6, 2020

trask approved these changes Oct 7, 2020

View reviewed changes

trask merged commit 9523f9f into open-telemetry:master Oct 7, 2020

imavroukakis deleted the fix-spotbugs-violations branch October 7, 2020 08:04

imavroukakis mentioned this pull request Oct 8, 2020

spotbugs upgrade and fixes for JMS1 tests #1346

Merged

iNikem mentioned this pull request Oct 14, 2020

Should we fix the SpotBugs violation? #1166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix and/or ignore spotbugs errors #1325

fix and/or ignore spotbugs errors #1325

imavroukakis commented Oct 5, 2020 •

edited

Loading

iNikem Oct 5, 2020

imavroukakis Oct 5, 2020 •

edited

Loading

iNikem Oct 5, 2020

imavroukakis Oct 5, 2020

trask Oct 5, 2020

iNikem commented Oct 5, 2020

mateuszrzeszutek commented Oct 5, 2020

imavroukakis commented Oct 5, 2020

imavroukakis commented Oct 5, 2020

mateuszrzeszutek commented Oct 5, 2020

imavroukakis commented Oct 5, 2020

iNikem commented Oct 5, 2020

imavroukakis commented Oct 5, 2020

iNikem commented Oct 5, 2020

anuraaga commented Oct 5, 2020

imavroukakis commented Oct 6, 2020

mateuszrzeszutek commented Oct 6, 2020

imavroukakis commented Oct 6, 2020 •

edited

Loading

mateuszrzeszutek commented Oct 6, 2020

iNikem commented Oct 6, 2020

imavroukakis commented Oct 6, 2020

iNikem commented Oct 6, 2020

imavroukakis commented Oct 6, 2020

iNikem commented Oct 6, 2020

imavroukakis commented Oct 6, 2020

imavroukakis commented Oct 6, 2020

imavroukakis commented Oct 6, 2020

iNikem commented Oct 6, 2020

trask left a comment

fix and/or ignore spotbugs errors #1325

fix and/or ignore spotbugs errors #1325

Conversation

imavroukakis commented Oct 5, 2020 • edited Loading

iNikem Oct 5, 2020

Choose a reason for hiding this comment

imavroukakis Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

iNikem Oct 5, 2020

Choose a reason for hiding this comment

imavroukakis Oct 5, 2020

Choose a reason for hiding this comment

trask Oct 5, 2020

Choose a reason for hiding this comment

iNikem commented Oct 5, 2020

mateuszrzeszutek commented Oct 5, 2020

imavroukakis commented Oct 5, 2020

imavroukakis commented Oct 5, 2020

mateuszrzeszutek commented Oct 5, 2020

imavroukakis commented Oct 5, 2020

iNikem commented Oct 5, 2020

imavroukakis commented Oct 5, 2020

iNikem commented Oct 5, 2020

anuraaga commented Oct 5, 2020

imavroukakis commented Oct 6, 2020

mateuszrzeszutek commented Oct 6, 2020

imavroukakis commented Oct 6, 2020 • edited Loading

mateuszrzeszutek commented Oct 6, 2020

iNikem commented Oct 6, 2020

imavroukakis commented Oct 6, 2020

iNikem commented Oct 6, 2020

imavroukakis commented Oct 6, 2020

iNikem commented Oct 6, 2020

imavroukakis commented Oct 6, 2020

imavroukakis commented Oct 6, 2020

imavroukakis commented Oct 6, 2020

iNikem commented Oct 6, 2020

trask left a comment

Choose a reason for hiding this comment

imavroukakis commented Oct 5, 2020 •

edited

Loading

imavroukakis Oct 5, 2020 •

edited

Loading

imavroukakis commented Oct 6, 2020 •

edited

Loading