Java test example, additional tags for actor count, sanitise tags for datadog

[hughsimpson]

This PR tests that included/excluded actor path filtering works for the java api with unnamed anonymousinner class actors, and solves the problem of not being able to group such actors when displaying actor count information in datadog. Also, it sanitises tags before sending data to datadog, so we don't get errors in data display.

[janm399]

Have you noticed that we can't automatically merge?

[janm399]

I can see progress. But, the code is very untidy. Unused imports, needless variables, bad formatting. Every detail counts.

[janm399]

What's the difference between JavaApiSpec and JavaApiFiltersSpec. They look mostly the same, can they be refactored?

[MarioArias]

Different filter configurations

[janm399]

More progress. Good. Now that you have the enum, you can have a switch, which is more idiomatic in that situation.

         switch (countType) {
             case Increment:
                 break;
             case Decrement:
                 break;
         }

I find it clumsy to have a private enum in the middle of (public) advices.

Next up, on line 252, don't use string concatenation, use String.format.

[janm399]

When does uncheckedActorNameFrom return null?

[janm399]

...and we failed the CI build :(

[archena]

Has this been tested on Alex's scenario?

[MarioArias]

Is an indeterministic behaviour when we use Thread.sleep

The test fails randomly between JDKs and Scala versions.

[janm399]

We'll need to change the test then. We can't have non-deterministic tests, because we'll start ignoring the failures. You know, oh, this test sometimes fails, it's OK.

[MarioArias]

Ok, working on that

[MarioArias]

@archena Yesterday we made a test, this is way we're making this changes.

This version cannot be tested until released

[janm399]

@hughsimpson, in the aspect, you have

final String uncheckedClassName = uncheckedActorNameFrom(props);
...

String checkedClassName;
if (uncheckedClassName == null) checkedClassName = "";
else checkedClassName = uncheckedClassName;

When does unchecledActorNameFrom(Props) return null?

Also, there are plenty of comments on private methods that look like JavaDocs, but are malformed. If you want just a comment, then don't start it with /**. A good example is

    /**
     * get the sample rate for an actor
     * */
    private int getSampleRate(final PathAndClass pathAndClass) {
        return this.agentConfiguration.sampling().getRate(pathAndClass);
    }

Finally, there's still the string concatenation using +.

this.counterInterface.recordGaugeValue("akka.actor.count", currentNumberOfActors,
    "parent."+path.parent().toString(),
    "path."+path.toString(),
    "props."+props.clazz().toString(),
    "className."+checkedClassName);

Use String.format, or, even better StringBuilder.append.

[MarioArias]

String concatenation addressed

[MarioArias]

Comments addressed

[janm399]

:star:

[janm399]

I've seen tens of (a, _) => a, can we have a val for it?

[janm399]

This is best illustrated in a screenshot.

Why have the unused variables? unnamedGreetPrinterTags and the other underlined three.

[archena]

So I'm trying to check the logic of include/exclude. It looks like

Actors belonging to the monitor are always excluded. If an actor is in the include list, it's included If an actor is not in the include list but is in the reject list, it's rejected If an actor is in neither the include nor exclude list and includeSystemAgents() is true, we include it (why?) Otherwise, it depends on the userOrSystem check

At line 72 ActorCellMonitoringAspect.aj things get confusing (or I get confused, at any rate!).

What is a 'system' actor? is it right to conclude, as this seems to, that if an actor isn't in either list, it is a system actor? what's the meaning of this comment:

// skip system actor checks if we wish to monitor them
if (this.agentConfiguration.includeSystemAgents()) return true;

Does that mean "The skip-system-actor is something which checks if we wish to monitor them" or "this skips system actor checks, if we wish to monitor them"? can it be clarified?

What is the intention of:

String userOrSystem = pathAndClass.actorPath().getElements().iterator().next();
return "user".equals(userOrSystem);

More broadly, what happens if an actor is not in either list? are you satisfied that the tests cover all of the above conditions? excuse the stupid questions but this kind of thing is difficult to get right!

[janm399]

Would it not make sense to have the takeLHS function in the TestCounter companion object? (Moreover sharedCode is not our favourite naming convention: top-level object with lower-case first char?)

[janm399]

We're getting closer. Now, the first 45 lines of the JavaApiSpec and JavaApiFiltersSpec are the same. It would be useful to have

abstract class ActorCellMonitoringAspectSpec(agentConfig: Option[String]) extends TestKit(ActorSystem()) with SpecificationLike {
  sequential

  protected final def configure(config: String): Unit = {
    val configuration = AkkaAgentConfiguration(ConfigFactory.load(s"META-INF/monitor/$config"))
    aspect.setAgentConfiguration(configuration)
  }

  val aspect: ActorCellMonitoringAspect = ActorCellMonitoringAspect.aspectOf()
  agentConfig.map(configure)

}

This allows you to call configure(String) in the tests, and therefore allows you to collapse the two files into one, sharing and re-using the code.

[janm399]

We are writing

props.class_org.eigengo.monitor.agent.akka.SimpleActor

to DD (with underscore), is that what we expect? Would we not want to see

``props.class.org.eigengo.monitor.agent.akka.SimpleActor

Would having a . after class make more sense and be more consistent with all other values?

And finally, isn't tag format tag:value? And so, should we not have akka:${whatever-follows}?