Post events from discovery server to ECAP that will allow me to generate the necessary histogram data #6

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
5 participants

wyan0220 commented Jul 9, 2012

... the necessary histogram data.

@electrum electrum commented on an outdated diff Jul 10, 2012

...ava/com/proofpoint/discovery/DynamicAnnouncement.java
@@ -157,4 +146,11 @@ public DynamicAnnouncement build()
return new DynamicAnnouncement(environment, pool, location, services);
}
}
+
+ @Override
+ public String toString()
+ {
+ return String.format("DynamicAnnouncement{environment:%s,location:%s,pool:%s,services:%s}", environment,
@electrum

electrum Jul 10, 2012

Use Guava Objects.toStringHelper

@electrum electrum commented on an outdated diff Jul 10, 2012

...om/proofpoint/discovery/monitor/DiscoveryMonitor.java
+ {
+ this.eventClient = Preconditions.checkNotNull(eventClient);
+ this.stats = Preconditions.checkNotNull(stats);
+ }
+
+ public void monitorDiscoveryEvent(DiscoveryEventType type, boolean success, String remoteAddress, String requestUri, String requestBodyJson, long startTime)
+ {
+ eventClient.post(new DiscoveryEvent(type, success, remoteAddress, requestUri,
+ requestBodyJson, new Duration(System.nanoTime() - startTime, TimeUnit.NANOSECONDS)));
+ stats.addStats(type, success, startTime);
+ }
+
+ public void monitorDiscoveryFailureEvent(DiscoveryEventType type, Exception e, String requestUri)
+ {
+ eventClient.post(new DiscoveryFailureEvent(type, e, requestUri));
+ log.warn(e, String.format("discovery request [%s] failed", requestUri));
@electrum

electrum Jul 10, 2012

You shouldn't log things that are then re-thrown. This causes duplicate log messages and is confusing.

@electrum electrum commented on an outdated diff Jul 10, 2012

.../com/proofpoint/discovery/monitor/DiscoveryStats.java
+ public TimedStat getDynamicAnnouncementProcessingTime()
+ {
+ return dynamicAnnouncementProcessingTime;
+ }
+
+ @Managed
+ @Nested
+ public TimedStat getDynamicAnnouncementDeleteProcessingTime()
+ {
+ return dynamicAnnouncementDeleteProcessingTime;
+ }
+
+ public void addStats(DiscoveryEventType type, boolean success, long startTime)
+ {
+ if (success) {
+ switch (type) {

@electrum electrum commented on an outdated diff Jul 10, 2012

.../com/proofpoint/discovery/monitor/DiscoveryStats.java
@@ -0,0 +1,205 @@
+package com.proofpoint.discovery.monitor;
+
+import com.proofpoint.stats.TimedStat;
+import com.proofpoint.units.Duration;
+import org.weakref.jmx.Managed;
+import org.weakref.jmx.Nested;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class DiscoveryStats
+{
+ private final AtomicLong serviceQuerySuccessCount = new AtomicLong();
@electrum

electrum Jul 10, 2012

There should be a way to use EnumMap for all of these and still have it work with JMX.

@electrum electrum and 2 others commented on an outdated diff Jul 10, 2012

...proofpoint/discovery/DynamicAnnouncementResource.java
{
- if (!nodeInfo.getEnvironment().equals(announcement.getEnvironment())) {
- return Response.status(BAD_REQUEST)
- .entity(format("Environment mismatch. Expected: %s, Provided: %s", nodeInfo.getEnvironment(), announcement.getEnvironment()))
- .build();
- }
+ boolean success = true;
@electrum

electrum Jul 10, 2012

This is ugly having all this logic in the resource. It's much cleaner to wrap the store and put the logic in the wrapper. You can also consider using a dynamic proxy (java.lang.reflect.Proxy) in the wrapper to eliminate boilerplate.

@anandsomani

anandsomani Jul 10, 2012

Member

Please ignore if you are commenting on the boiler plate code for events. I thought this was on the environment check...

Generally I agree that business logic belongs in store and not in resource. I have seen a tendency to add business logic to resource, which is a bad idea. Resource is just one way of exposing this API and should only contain API to internal <=> mapping.

In this case, I cannot pin point why, but I feel leaving this check here makes more sense since this in someways is the API contract.

@electrum

electrum Jul 11, 2012

I was referring to all the stats logic, not the environment mismatch check. The check is fine.

@wyan0220

wyan0220 Jul 13, 2012

hi David, i implemented a wrapper class for event monitoring logic. could you kindly review it? Thanks

@wyan0220 wyan0220 changes based on review:
1. use guava toStringHelper
2. use EnumMap
5577c22
Owner

wyan0220 commented on 5577c22 Jul 10, 2012

Hi David, thanks for your comments. I've made the changes for most of them. Regarding dynamic proxy, i was told we should stay away from it if possible. For me I feel like it's a little overkill in this case...

This check doesn't seem necessary: it's fine to pass null to toStringHelper. If you do want it to be displayed as an empty string rather than null, then use Strings.nullToEmpty.

It's cleaner to call this eventTypeStats and to make the type Map rather than EnumMap. Always prefer to use the most generic collection class when declaring variables (e.g. List vs ArrayList or Map vs HashMap).

It can make the code a little shorter and easier to read if you static import the enum values.

hi David, i implemented a wrapper class for event monitoring logic. could you kindly review it? Thanks

Owner

wyan0220 replied Jul 18, 2012

instead of monitor() maybe use execute(), since the purpose is not to monitor but to execute something that is being monitored

@markkent markkent commented on an outdated diff Jul 17, 2012

.../java/com/proofpoint/discovery/EventMonitorProxy.java
+ private final UriInfo uriInfo;
+ private final HttpServletRequest httpServletRequest;
+ private final String requestBody;
+
+ public EventMonitorProxy(DiscoveryMonitor discoveryMonitor, DiscoveryEventType type, UriInfo uriInfo, HttpServletRequest httpServletRequest, String requestBody)
+ {
+ this.discoveryMonitor = discoveryMonitor;
+ this.type = type;
+ this.uriInfo = uriInfo;
+ this.httpServletRequest = httpServletRequest;
+ this.requestBody = requestBody;
+ }
+
+ public T execute()
+ {
+ boolean success = true;
@markkent

markkent Jul 17, 2012

Prefer the opposite logic here. For example, if your code throws an Error, you'll log a success event. Better to initialize success to false, and set it to true after doWork returns.

@markkent markkent commented on an outdated diff Jul 17, 2012

...proofpoint/discovery/DynamicAnnouncementResource.java
{
- if (!nodeInfo.getEnvironment().equals(announcement.getEnvironment())) {
- return Response.status(BAD_REQUEST)
- .entity(format("Environment mismatch. Expected: %s, Provided: %s", nodeInfo.getEnvironment(), announcement.getEnvironment()))
- .build();
- }
+ EventMonitorProxy<Response> eventMonitor = new EventMonitorProxy<Response>(discoveryMonitor, DiscoveryEventType.DYNAMICANNOUNCEMENT, uriInfo, httpServletRequest, announcement.toString())
+ {
+ @Override
+ public Response doWork()
+ {
+ if (!nodeInfo.getEnvironment().equals(announcement.getEnvironment())) {
+ return Response.status(BAD_REQUEST)
+ .entity(format("Environment mismatch. Expected: %s, Provided: %s", nodeInfo.getEnvironment(), announcement.getEnvironment()))
@markkent

markkent Jul 17, 2012

In the event of this BAD_REQUEST result, you will still log a success event in the monitor. Is that really what you want?

@markkent markkent commented on the diff Jul 17, 2012

.../proofpoint/discovery/monitor/DiscoveryEventType.java
@@ -0,0 +1,11 @@
+package com.proofpoint.discovery.monitor;
+
+public enum DiscoveryEventType
@markkent

markkent Jul 17, 2012

Can't you make this the event object? Seems like it would be neater.

@markkent markkent commented on the diff Jul 17, 2012

.../com/proofpoint/discovery/monitor/DiscoveryEvent.java
@@ -0,0 +1,127 @@
+package com.proofpoint.discovery.monitor;
+
+import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import com.proofpoint.event.client.EventField;
+import com.proofpoint.event.client.EventType;
+import com.proofpoint.units.Duration;
+
+@EventType("platform:type=discovery,name=discovery")
@markkent

markkent Jul 17, 2012

We're going to end up bifurcating discovery because of this - this event type name isn't consistent with the naming scheme for v2 events.

@markkent markkent commented on an outdated diff Jul 17, 2012

.../proofpoint/discovery/StaticAnnouncementResource.java
{
- return new Services(nodeInfo.getEnvironment(), store.getAll());
+ EventMonitorProxy<Services> eventMonitor = new EventMonitorProxy<Services>(discoveryMonitor, DiscoveryEventType.STATICANNOUNCEMENTLIST, uriInfo, httpServletRequest, "")
@markkent

markkent Jul 17, 2012

Consider adding static factory methods to make this code easier to read.

@markkent markkent commented on an outdated diff Jul 17, 2012

...proofpoint/discovery/DynamicAnnouncementResource.java
{
- if (!dynamicStore.delete(nodeId)) {
- return Response.status(NOT_FOUND).build();
- }
+ EventMonitorProxy<Response> eventMonitor = new EventMonitorProxy<Response>(discoveryMonitor, DiscoveryEventType.DYNAMICANNOUNCEMENTDELETE, uriInfo, httpServletRequest, "")
@markkent

markkent Jul 17, 2012

This still looks kinda ugly. Is there a way to make it neater?

@markkent markkent commented on the diff Jul 18, 2012

.../proofpoint/discovery/monitor/MonitorInterceptor.java
@@ -0,0 +1,40 @@
+package com.proofpoint.discovery.monitor;
+
+import com.google.inject.Inject;
+import org.aopalliance.intercept.MethodInterceptor;
+import org.aopalliance.intercept.MethodInvocation;
+
+import javax.servlet.http.HttpServletRequest;
+import javax.ws.rs.core.UriInfo;
+
+public class MonitorInterceptor implements MethodInterceptor
+{
+ @Inject
+ DiscoveryMonitor discoveryMonitor;
@markkent

markkent Jul 18, 2012

Is field injection the only way to get this to work? I'm guessing so. Shame

@markkent markkent commented on an outdated diff Jul 18, 2012

.../proofpoint/discovery/monitor/MonitorInterceptor.java
+
+import javax.servlet.http.HttpServletRequest;
+import javax.ws.rs.core.UriInfo;
+
+public class MonitorInterceptor implements MethodInterceptor
+{
+ @Inject
+ DiscoveryMonitor discoveryMonitor;
+
+ @Override
+ public Object invoke(MethodInvocation methodInvocation)
+ throws Throwable
+ {
+ boolean success = false;
+ long startTime = System.nanoTime();
+ DiscoveryEventType type = methodInvocation.getMethod().getAnnotation(MonitorWith.class).value();
@markkent

markkent Jul 18, 2012

Hrm... I haven't really looked at this, but we probably don't want to be reading the annotations every time through this code if we can avoid it. Wouldn't that be wasteful?

Also, all this getArguments()[index] looks odd.

I need to read up on how this junk works.

@markkent markkent commented on an outdated diff Jul 18, 2012

...a/com/proofpoint/discovery/DiscoveryServerModule.java
@@ -62,6 +69,15 @@ public void configure(Binder binder)
binder.bind(StaticStore.class).to(ReplicatedStaticStore.class).in(Scopes.SINGLETON);
binder.install(new ReplicatedStoreModule("static", ForStaticStore.class, PersistentStore.class));
bindConfig(binder).prefixedWith("static").to(PersistentStoreConfig.class);
+
+ // event
+ eventBinder(binder).bindEventClient(DiscoveryEvent.class);
+ eventBinder(binder).bindEventClient(DiscoveryFailureEvent.class);
+
+ // interceptor
+ MonitorInterceptor interceptor = new MonitorInterceptor();
+ binder.requestInjection(interceptor);
+ binder.bindInterceptor(Matchers.any(), Matchers.annotatedWith(MonitorWith.class), interceptor);
@markkent

markkent Jul 18, 2012

A couple of things here. Firstly, put the monitor-related things in a module that you bind separately.

Also, perhaps also require the class to be annotated (perhaps with @monitored, and then the methods with @monitorwithevent()) Maybe not. I'm not sure.

@markkent markkent commented on an outdated diff Jul 18, 2012

...proofpoint/discovery/DynamicAnnouncementResource.java
@@ -49,7 +53,8 @@ public DynamicAnnouncementResource(DynamicStore dynamicStore, NodeInfo nodeInfo)
@PUT
@Consumes(MediaType.APPLICATION_JSON)
- public Response put(@PathParam("node_id") Id<Node> nodeId, @Context UriInfo uriInfo, DynamicAnnouncement announcement)
+ @MonitorWith(DYNAMICANNOUNCEMENT)
@markkent

markkent Jul 18, 2012

yeah, I don't like the way "MonitorWith" reads here. (yes, I know it was my name) Find a better name for the annotation? At the very least, the annotations appear to be stating properties of the method, so "MonitoredWith" seems better.

@markkent markkent commented on an outdated diff Jul 18, 2012

...in/java/com/proofpoint/discovery/ServiceResource.java
{
return new Services(node.getEnvironment(), union(dynamicStore.get(type), staticStore.get(type)));
}
@GET
@Produces(MediaType.APPLICATION_JSON)
- public Services getServices()
+ @MonitorWith(SERVICEQUERY)
+ public Services getServices(@Context HttpServletRequest httpServletRequest, @Context UriInfo uriInfo)
@markkent

markkent Jul 18, 2012

I think it's really really clunky that you have to add these two parameters, which the IDE will claim aren't used in the function, just because the interceptor expects to rely on their value. Is there a better way of doing this?

Owner

johngmyers commented Oct 17, 2013

Not going to use events for instrumenting discovery server. Closing out.

johngmyers closed this Oct 17, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment