Skip to content
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

Specify sampling rate for actors #56

Merged
merged 21 commits into from
Nov 6, 2013

Conversation

hughsimpson
Copy link
Contributor

No description provided.

@hughsimpson hughsimpson mentioned this pull request Nov 4, 2013

"Sample every 'n' messages (where 'n' is defined in conf file)" in {
TestCounterInterface.clear()
(0 until 1000) map {_ => a ! 1}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably use foreach. The effect will be the same, but the name map is a bit loaded.

@janm399
Copy link
Member

janm399 commented Nov 4, 2013

I think it's time to refactor the def accept(actorPath: ActorPath, actorClassName: Option[String]): Boolean into something that composes a bit better. I've seen it duplicated in this PR too often; the duplication is either pretty obvious (same name and params) or slightly hidden (viz includes and accept).

Let's tidy that up.

@@ -39,4 +42,14 @@ abstract aspect AbstractMonitoringAspect {
protected final <A> AgentConfiguration<A> getAgentConfiguration(String agentName, Function1<Config, A> agent) {
return AgentConfigurationFactory.getAgentCofiguration(agentName, agent);
}

protected final HashMap<ActorFilter, AtomicLong> getCounterKeys(AgentConfiguration<AkkaAgentConfiguration> configuration) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be in the subclass. I'm beginning to think that the abstract aspect might not be needed at all.

@janm399
Copy link
Member

janm399 commented Nov 5, 2013

This is looking much better. The only thing is that the sampling rate skews the actual number of messages. You send the actor 1000 messages with sampling rate 5, but the counter value is only 200. It should read 1000, but with 5 increments.

There's a whole bunch of missing ScalaDoc, unused imports, and the naming of the tests does not really match the existing tests.

Finally, the WithUnhandledActor.scala contains too many actors. It may be worth pulling all test actors into one file, for example actors.scala

private def parseSampling(samplingObject: ConfigObject): Iterable[SamplingRate] = {
import scala.collection.JavaConversions._
(samplingObject.get("rate").unwrapped(), samplingObject.get("for").unwrapped()) match {
case (rate: Number, filters: java.util.List[String @unchecked]) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I didn't know about that @unchcked annotation.

@janm399
Copy link
Member

janm399 commented Nov 5, 2013

Looking good. Just a few last bits to tidy up and we're good to merge.

@janm399
Copy link
Member

janm399 commented Nov 5, 2013

I pulled the latest code. Sbt builds properly, scalastyle reports no errors. That is actually a problem, because PathAndClass is still missing documentation and the Scaladoc for ActorFilter.accept is not correct. (Even the IDE shows it in red!)

The code in the ActorCellMonitoringAspect is good, but you should pay attention to the code style. All fields are accessed using this., for example:

        if (!this.agentConfiguration.incuded().accept(pathAndClass)) return false;
        if (this.agentConfiguration.excluded().accept(pathAndClass)) return false;

        if (this.agentConfiguration.includeSystemAgents()) return true;

Your code misses out this., making it look untidy. (Viz lines 69, 77, 78.)


Looking further, remember our rule to leave the place in a better state than it was when you found it. So, the documentation on the advices is poor. It repeats the documentation of the pointcut, but doesn't really explain what the advice does.

@janm399
Copy link
Member

janm399 commented Nov 5, 2013

As a result of the code review, we have two improvement issues: #59 and #60.

janm399 added a commit that referenced this pull request Nov 6, 2013
@janm399 janm399 merged commit f8aecb7 into eigengo:master Nov 6, 2013
@hughsimpson hughsimpson deleted the specify-actors-to-monitor2 branch November 6, 2013 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants