[GitHub] accumulo pull request #291: ACCUMULO-4693: Add instance-specific ProcessName...

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #291: ACCUMULO-4693: Add instance-specific ProcessName...

ctubbsii
GitHub user billoley opened a pull request:

    https://github.com/apache/accumulo/pull/291

    ACCUMULO-4693: Add instance-specific ProcessName to Hadoop2 metrics

    Create process name from system property -Dapp and instance from -Daccumulo.service.instance if applicable
   
    By adding the ProcessName tag to the MetricsRegistry, this tag will be added to all created metrics,
    allowing metrics consumers to deconflict metrics from different service instances

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/billoley/accumulo ACCUMULO-4693

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/accumulo/pull/291.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #291
   
----
commit 4d76acf2c5402a2a7c2982e298b3b5d983e47c43
Author: Bill Oley <[hidden email]>
Date:   2017-08-07T16:51:33Z

    ACCUMULO-4693: Add instance-specific ProcessName to Hadoop2 metrics
   
    Create process name from system property -Dapp and instance from -Daccumulo.service.instance if applicable
    By adding the ProcessName tag to the MetricsRegistry, this tag will be added to all created metrics,
    allowing metrics consumers to deconflict metrics from different service instances
   
    ACCUMULO-4693: Add instance-specific ProcessName to Hadoop2 metrics - code format

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #291: ACCUMULO-4693: Add instance-specific ProcessName to Had...

ctubbsii
Github user billoley commented on the issue:

    https://github.com/apache/accumulo/pull/291
 
    I'd be happy to use something other than the system properties and am open to suggestions.
   
    The need for discovering the instance (1,2, etc) is that intermingling tserver metrics makes the metrics pretty useless.   For metrics systems that auto-discover the existing tags (Master, TabletServer [for single instance servers], TabletServer-1, TabletServer-2, etc) per host, appending a randomly generated number would be much less elegant and would clutter displays and databases.
   
    The need for discovering the service name (-Dapp) comes from the Thrift metrics which are started with a thrift server on several different services.  To make these metrics useful, we need to be able to identify which application they came from.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #291: ACCUMULO-4693: Add instance-specific ProcessName to Had...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on the issue:

    https://github.com/apache/accumulo/pull/291
 
    @billoley What about just including all system properties, so it still supports this use case, but the code isn't tied to a specific launch script implementation? Is it possible to pick out just the specific system-property-turned-metric by the metrics consumer?
   
    The other thing I was thinking is that maybe we could tie it to a one-up number from the ZooKeeper lock.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #291: ACCUMULO-4693: Add instance-specific ProcessName to Had...

ctubbsii
In reply to this post by ctubbsii
Github user billoley commented on the issue:

    https://github.com/apache/accumulo/pull/291
 
    @ctubbsii
   
    I don't need to rely on the -Dapp property because the same string is
    declared in main() of TabletServer, Master, etc and passed to various init
    methods.  I can configure the MetricsSystemHelper from the same entry point.
   
    For the tablet server instance, I could cycle through the system properties
    and use the first one that includes the word "instance" defaulting to empty
    string.  This would decouple the code from a specific property while still
    allowing users to specify an instance to deconflict metrics.  Is this a
    better solution?
   
   
   
   
   
   
    Bill Oley
   
    On Mon, Aug 7, 2017 at 6:08 PM, Christopher Tubbs <[hidden email]>
    wrote:
   
    > @billoley <https://github.com/billoley> What about just including all
    > system properties, so it still supports this use case, but the code isn't
    > tied to a specific launch script implementation? Is it possible to pick out
    > just the specific system-property-turned-metric by the metrics consumer?
    >
    > The other thing I was thinking is that maybe we could tie it to a one-up
    > number from the ZooKeeper lock.
    >
    > —
    > You are receiving this because you were mentioned.
    > Reply to this email directly, view it on GitHub
    > <https://github.com/apache/accumulo/pull/291#issuecomment-320794375>, or mute
    > the thread
    > <https://github.com/notifications/unsubscribe-auth/AFVhw1fclLHN2b0AJsnzqu4jccwPjJJYks5sV4rkgaJpZM4Ovy7u>
    > .
    >



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #291: ACCUMULO-4693: Add instance-specific ProcessName to Had...

ctubbsii
In reply to this post by ctubbsii
Github user mikewalch commented on the issue:

    https://github.com/apache/accumulo/pull/291
 
    > Thoughts, @mikewalch ?
   
    In Accumulo 2.0, the system property `app` is no longer set. Instead, a system property `accumulo.application` is set in [accumulo-env.sh](https://github.com/apache/accumulo/blob/master/assemble/conf/accumulo-env.sh#L93) to something like `tserver_myhost` or `tserver2_myhost` if multiple instances of a service are running.  The property `accumulo.application` is used to configure log filenames and identify the application sending logs to monitor.  Below are uses of it in codebase:
   
    ```
    ./assemble/conf/accumulo-env.sh:  "-Daccumulo.application=${cmd}${ACCUMULO_SERVICE_INSTANCE}_$(hostname)")
    ./assemble/conf/log4j-monitor.properties:log4j.appender.file.File=${accumulo.log.dir}/${accumulo.application}.log
    ./assemble/conf/log4j-service.properties:log4j.appender.file.File=${accumulo.log.dir}/${accumulo.application}.log
    ./assemble/conf/log4j-service.properties:#log4j.appender.audit.File=${accumulo.log.dir}/${accumulo.application}.audit
    ./server/monitor/src/main/java/org/apache/accumulo/monitor/util/AccumuloMonitorAppender.java:      socketAppender.setApplication(System.getProperty("accumulo.application", "unknown"));
    ```
   
    I wanted to point the existence of `accumulo.application` but I am not saying it should be used for this use case.  It makes sense for log4j but I can see why @ctubbsii wants to get away from its use in Java.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #291: ACCUMULO-4693: Add instance-specific ProcessName to Had...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on the issue:

    https://github.com/apache/accumulo/pull/291
 
    @billoley I had forgotten that we already define `accumulo.application` system property (optional) in master for the AccumuloMonitorAppender. I suppose it's not so bad if we consistently use that property to identify the service, as long as the behavior is reasonable if the property is not set (for example, a random number generated at server start).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #291: ACCUMULO-4693: Add instance-specific ProcessName...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/291#discussion_r132548358
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsSystemHelper.java ---
    @@ -16,20 +16,65 @@
      */
     package org.apache.accumulo.server.metrics;
     
    +import java.util.HashMap;
    +import java.util.Map;
    +
     import org.apache.hadoop.metrics2.MetricsSystem;
     import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
    +import org.apache.hadoop.metrics2.source.JvmMetrics;
    +import org.apache.hadoop.metrics2.source.JvmMetricsInfo;
     
     /**
      *
      */
     public class MetricsSystemHelper {
     
    +  private static Map<String,String> serviceNameMap = new HashMap<>();
    +  private static String processName = "Unknown";
    +
    +  static {
    +    serviceNameMap.put("master", "Master");
    +    serviceNameMap.put("tserver", "TabletServer");
    +    serviceNameMap.put("monitor", "Monitor");
    +    serviceNameMap.put("gc", "GarbageCollector");
    +    serviceNameMap.put("tracer", "Tracer");
    +    serviceNameMap.put("shell", "Shell");
    --- End diff --
   
    What is this mapping for? Seems strange to do it here.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #291: ACCUMULO-4693: Add instance-specific ProcessName...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/291#discussion_r132547937
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsSystemHelper.java ---
    @@ -16,20 +16,65 @@
      */
     package org.apache.accumulo.server.metrics;
     
    +import java.util.HashMap;
    +import java.util.Map;
    +
     import org.apache.hadoop.metrics2.MetricsSystem;
     import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
    +import org.apache.hadoop.metrics2.source.JvmMetrics;
    +import org.apache.hadoop.metrics2.source.JvmMetricsInfo;
     
     /**
      *
      */
     public class MetricsSystemHelper {
     
    +  private static Map<String,String> serviceNameMap = new HashMap<>();
    +  private static String processName = "Unknown";
    +
    +  static {
    +    serviceNameMap.put("master", "Master");
    +    serviceNameMap.put("tserver", "TabletServer");
    +    serviceNameMap.put("monitor", "Monitor");
    +    serviceNameMap.put("gc", "GarbageCollector");
    +    serviceNameMap.put("tracer", "Tracer");
    +    serviceNameMap.put("shell", "Shell");
    +  }
    +
    +  public static void configure(String application) {
    +    String serviceName = application;
    +    if (MetricsSystemHelper.serviceNameMap.containsKey(application)) {
    +      serviceName = MetricsSystemHelper.serviceNameMap.get(application);
    +    }
    +
    +    // a system property containing 'instance' can be used if more than one TabletServer is started on a host
    +    String serviceInstance = "";
    +    if (serviceName.equals("TabletServer")) {
    +      for (Map.Entry<Object,Object> p : System.getProperties().entrySet()) {
    +        if (((String) p.getKey()).contains("instance")) {
    +          // get rid of all non-alphanumeric characters and then prefix with a -
    +          serviceInstance = "-" + ((String) p.getValue()).replaceAll("[^a-zA-Z0-9]", "");
    --- End diff --
   
    Is this transformation necessary to conform to restriction of the metrics library?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #291: ACCUMULO-4693: Add instance-specific ProcessName...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/291#discussion_r132547783
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsSystemHelper.java ---
    @@ -16,20 +16,65 @@
      */
     package org.apache.accumulo.server.metrics;
     
    +import java.util.HashMap;
    +import java.util.Map;
    +
     import org.apache.hadoop.metrics2.MetricsSystem;
     import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
    +import org.apache.hadoop.metrics2.source.JvmMetrics;
    +import org.apache.hadoop.metrics2.source.JvmMetricsInfo;
     
     /**
      *
      */
     public class MetricsSystemHelper {
     
    +  private static Map<String,String> serviceNameMap = new HashMap<>();
    +  private static String processName = "Unknown";
    +
    +  static {
    +    serviceNameMap.put("master", "Master");
    +    serviceNameMap.put("tserver", "TabletServer");
    +    serviceNameMap.put("monitor", "Monitor");
    +    serviceNameMap.put("gc", "GarbageCollector");
    +    serviceNameMap.put("tracer", "Tracer");
    +    serviceNameMap.put("shell", "Shell");
    +  }
    +
    +  public static void configure(String application) {
    +    String serviceName = application;
    +    if (MetricsSystemHelper.serviceNameMap.containsKey(application)) {
    +      serviceName = MetricsSystemHelper.serviceNameMap.get(application);
    +    }
    +
    +    // a system property containing 'instance' can be used if more than one TabletServer is started on a host
    +    String serviceInstance = "";
    +    if (serviceName.equals("TabletServer")) {
    +      for (Map.Entry<Object,Object> p : System.getProperties().entrySet()) {
    +        if (((String) p.getKey()).contains("instance")) {
    --- End diff --
   
    What's the rationale for this? Pattern matching? This seems prone to errors, picking up the wrong property.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #291: ACCUMULO-4693: Add instance-specific ProcessName...

ctubbsii
In reply to this post by ctubbsii
Github user billoley commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/291#discussion_r132674184
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsSystemHelper.java ---
    @@ -16,20 +16,65 @@
      */
     package org.apache.accumulo.server.metrics;
     
    +import java.util.HashMap;
    +import java.util.Map;
    +
     import org.apache.hadoop.metrics2.MetricsSystem;
     import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
    +import org.apache.hadoop.metrics2.source.JvmMetrics;
    +import org.apache.hadoop.metrics2.source.JvmMetricsInfo;
     
     /**
      *
      */
     public class MetricsSystemHelper {
     
    +  private static Map<String,String> serviceNameMap = new HashMap<>();
    +  private static String processName = "Unknown";
    +
    +  static {
    +    serviceNameMap.put("master", "Master");
    +    serviceNameMap.put("tserver", "TabletServer");
    +    serviceNameMap.put("monitor", "Monitor");
    +    serviceNameMap.put("gc", "GarbageCollector");
    +    serviceNameMap.put("tracer", "Tracer");
    +    serviceNameMap.put("shell", "Shell");
    +  }
    +
    +  public static void configure(String application) {
    +    String serviceName = application;
    +    if (MetricsSystemHelper.serviceNameMap.containsKey(application)) {
    +      serviceName = MetricsSystemHelper.serviceNameMap.get(application);
    +    }
    +
    +    // a system property containing 'instance' can be used if more than one TabletServer is started on a host
    +    String serviceInstance = "";
    +    if (serviceName.equals("TabletServer")) {
    +      for (Map.Entry<Object,Object> p : System.getProperties().entrySet()) {
    +        if (((String) p.getKey()).contains("instance")) {
    +          // get rid of all non-alphanumeric characters and then prefix with a -
    +          serviceInstance = "-" + ((String) p.getValue()).replaceAll("[^a-zA-Z0-9]", "");
    --- End diff --
   
    No.  An underscore is added to the instance for the benefit of log filename formatting.  In the last iteration, I just removed the underscore.   I think the number without underscore looks more professional / less hack-ish when it shows up in the metrics repeatedly.  After I genericized the property (probably a mistake), I added this to remove non alpha/digit characters.  The leading dash I can take or leave.   (TabletServer-1 vs TabletServer1)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #291: ACCUMULO-4693: Add instance-specific ProcessName...

ctubbsii
In reply to this post by ctubbsii
Github user billoley commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/291#discussion_r132674187
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsSystemHelper.java ---
    @@ -16,20 +16,65 @@
      */
     package org.apache.accumulo.server.metrics;
     
    +import java.util.HashMap;
    +import java.util.Map;
    +
     import org.apache.hadoop.metrics2.MetricsSystem;
     import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
    +import org.apache.hadoop.metrics2.source.JvmMetrics;
    +import org.apache.hadoop.metrics2.source.JvmMetricsInfo;
     
     /**
      *
      */
     public class MetricsSystemHelper {
     
    +  private static Map<String,String> serviceNameMap = new HashMap<>();
    +  private static String processName = "Unknown";
    +
    +  static {
    +    serviceNameMap.put("master", "Master");
    +    serviceNameMap.put("tserver", "TabletServer");
    +    serviceNameMap.put("monitor", "Monitor");
    +    serviceNameMap.put("gc", "GarbageCollector");
    +    serviceNameMap.put("tracer", "Tracer");
    +    serviceNameMap.put("shell", "Shell");
    +  }
    +
    +  public static void configure(String application) {
    +    String serviceName = application;
    +    if (MetricsSystemHelper.serviceNameMap.containsKey(application)) {
    +      serviceName = MetricsSystemHelper.serviceNameMap.get(application);
    +    }
    +
    +    // a system property containing 'instance' can be used if more than one TabletServer is started on a host
    +    String serviceInstance = "";
    +    if (serviceName.equals("TabletServer")) {
    +      for (Map.Entry<Object,Object> p : System.getProperties().entrySet()) {
    +        if (((String) p.getKey()).contains("instance")) {
    --- End diff --
   
    This was an attempt to not tie the code to a specifc property.  It's probably better to just use the property.   I can't just pass all of the properties to the metrics system and defer the decision.  I'm creating a specific tag "ProcessName" that is used by the MetricsRegistry and metrics2 sinks.  We are using the -Daccumulo.service.instance to deconflict the log file names (this is why the _ is added to the instance) and to correlate a running process to the logs.  I'm trying to make it easy to montor the long-running tserver processes by using well-named metrics that also correlate to the running process and the log file with the same instance (1, 2, 3, etc).  Since this is only a concern for the tservers, I could grab this property closer to the top of the call chain (TabletServer.main()) if that would be more acceptable.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #291: ACCUMULO-4693: Add instance-specific ProcessName...

ctubbsii
In reply to this post by ctubbsii
Github user billoley commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/291#discussion_r132674190
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsSystemHelper.java ---
    @@ -16,20 +16,65 @@
      */
     package org.apache.accumulo.server.metrics;
     
    +import java.util.HashMap;
    +import java.util.Map;
    +
     import org.apache.hadoop.metrics2.MetricsSystem;
     import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
    +import org.apache.hadoop.metrics2.source.JvmMetrics;
    +import org.apache.hadoop.metrics2.source.JvmMetricsInfo;
     
     /**
      *
      */
     public class MetricsSystemHelper {
     
    +  private static Map<String,String> serviceNameMap = new HashMap<>();
    +  private static String processName = "Unknown";
    +
    +  static {
    +    serviceNameMap.put("master", "Master");
    +    serviceNameMap.put("tserver", "TabletServer");
    +    serviceNameMap.put("monitor", "Monitor");
    +    serviceNameMap.put("gc", "GarbageCollector");
    +    serviceNameMap.put("tracer", "Tracer");
    +    serviceNameMap.put("shell", "Shell");
    --- End diff --
   
    I was originally using this map when I grabbed the -Dapp system property.  Because the same string exists in the main() method of master, tserver, monitor, gc, and tracer, I changed to passing that string instead of grabbing the property.  I suppose I should just go one step further and pass the string that I want from each location which would get rid of this map.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #291: ACCUMULO-4693: Add instance-specific ProcessName...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/291#discussion_r133086237
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsSystemHelper.java ---
    @@ -16,20 +16,65 @@
      */
     package org.apache.accumulo.server.metrics;
     
    +import java.util.HashMap;
    +import java.util.Map;
    +
     import org.apache.hadoop.metrics2.MetricsSystem;
     import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
    +import org.apache.hadoop.metrics2.source.JvmMetrics;
    +import org.apache.hadoop.metrics2.source.JvmMetricsInfo;
     
     /**
      *
      */
     public class MetricsSystemHelper {
     
    +  private static Map<String,String> serviceNameMap = new HashMap<>();
    +  private static String processName = "Unknown";
    +
    +  static {
    +    serviceNameMap.put("master", "Master");
    +    serviceNameMap.put("tserver", "TabletServer");
    +    serviceNameMap.put("monitor", "Monitor");
    +    serviceNameMap.put("gc", "GarbageCollector");
    +    serviceNameMap.put("tracer", "Tracer");
    +    serviceNameMap.put("shell", "Shell");
    +  }
    +
    +  public static void configure(String application) {
    +    String serviceName = application;
    +    if (MetricsSystemHelper.serviceNameMap.containsKey(application)) {
    +      serviceName = MetricsSystemHelper.serviceNameMap.get(application);
    +    }
    +
    +    // a system property containing 'instance' can be used if more than one TabletServer is started on a host
    +    String serviceInstance = "";
    +    if (serviceName.equals("TabletServer")) {
    +      for (Map.Entry<Object,Object> p : System.getProperties().entrySet()) {
    +        if (((String) p.getKey()).contains("instance")) {
    --- End diff --
   
    I'm not sure it matters where in the call chain it's grabbed. I don't really know much about the Hadoop Metrics library. I was thinking maybe you could create a "SystemProperties" tag or something like that, and then on the client side of the metrics, pull out the specific property you need. That way, the only thing tied to the specific property was the launch script and the metrics consumer... and not any part of Accumulo's code.
   
    However, if that is not feasible for whatever reason, using the specific flag, if present, seems perfectly appropriate to me to me. It's basically what we did for the monitor log appender.
   
    If you wanted to push the service name implementation into the TabletServer, rather than the metrics code, that would work, too. Could create a method, `tabletServer.getServiceName()` or similar, to place the property-retrieval implementation.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #291: ACCUMULO-4693: Add instance-specific ProcessName...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/291#discussion_r133086488
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsSystemHelper.java ---
    @@ -16,20 +16,65 @@
      */
     package org.apache.accumulo.server.metrics;
     
    +import java.util.HashMap;
    +import java.util.Map;
    +
     import org.apache.hadoop.metrics2.MetricsSystem;
     import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
    +import org.apache.hadoop.metrics2.source.JvmMetrics;
    +import org.apache.hadoop.metrics2.source.JvmMetricsInfo;
     
     /**
      *
      */
     public class MetricsSystemHelper {
     
    +  private static Map<String,String> serviceNameMap = new HashMap<>();
    +  private static String processName = "Unknown";
    +
    +  static {
    +    serviceNameMap.put("master", "Master");
    +    serviceNameMap.put("tserver", "TabletServer");
    +    serviceNameMap.put("monitor", "Monitor");
    +    serviceNameMap.put("gc", "GarbageCollector");
    +    serviceNameMap.put("tracer", "Tracer");
    +    serviceNameMap.put("shell", "Shell");
    --- End diff --
   
    If the name represents a sensible self-identifier, it seems reasonable to provide it from within each service so they can identify themselves.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #291: ACCUMULO-4693: Add instance-specific ProcessName...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/291#discussion_r133086736
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsSystemHelper.java ---
    @@ -16,20 +16,65 @@
      */
     package org.apache.accumulo.server.metrics;
     
    +import java.util.HashMap;
    +import java.util.Map;
    +
     import org.apache.hadoop.metrics2.MetricsSystem;
     import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
    +import org.apache.hadoop.metrics2.source.JvmMetrics;
    +import org.apache.hadoop.metrics2.source.JvmMetricsInfo;
     
     /**
      *
      */
     public class MetricsSystemHelper {
     
    +  private static Map<String,String> serviceNameMap = new HashMap<>();
    +  private static String processName = "Unknown";
    +
    +  static {
    +    serviceNameMap.put("master", "Master");
    +    serviceNameMap.put("tserver", "TabletServer");
    +    serviceNameMap.put("monitor", "Monitor");
    +    serviceNameMap.put("gc", "GarbageCollector");
    +    serviceNameMap.put("tracer", "Tracer");
    +    serviceNameMap.put("shell", "Shell");
    +  }
    +
    +  public static void configure(String application) {
    +    String serviceName = application;
    +    if (MetricsSystemHelper.serviceNameMap.containsKey(application)) {
    +      serviceName = MetricsSystemHelper.serviceNameMap.get(application);
    +    }
    +
    +    // a system property containing 'instance' can be used if more than one TabletServer is started on a host
    +    String serviceInstance = "";
    +    if (serviceName.equals("TabletServer")) {
    +      for (Map.Entry<Object,Object> p : System.getProperties().entrySet()) {
    +        if (((String) p.getKey()).contains("instance")) {
    +          // get rid of all non-alphanumeric characters and then prefix with a -
    +          serviceInstance = "-" + ((String) p.getValue()).replaceAll("[^a-zA-Z0-9]", "");
    --- End diff --
   
    I think it'd be better to simply pass through what was put in the property, rather than parse in code what is provided in the process' environment. It makes too many assumptions about the launch environment inside code. If a specific format is desired, the launch script/environment should be tweaked to use the desired format.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #291: ACCUMULO-4693: Add instance-specific ProcessName...

ctubbsii
In reply to this post by ctubbsii
Github user mjwall commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/291#discussion_r133198698
 
    --- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/Metrics2TabletServerMinCMetrics.java ---
    @@ -17,10 +17,9 @@
     package org.apache.accumulo.tserver.metrics;
     
     import org.apache.accumulo.server.metrics.Metrics;
    -import org.apache.hadoop.metrics2.MetricsCollector;
    -import org.apache.hadoop.metrics2.MetricsRecordBuilder;
    -import org.apache.hadoop.metrics2.MetricsSource;
    -import org.apache.hadoop.metrics2.MetricsSystem;
    +import org.apache.accumulo.server.metrics.MetricsSystemHelper;
    +import org.apache.hadoop.metrics2.*;
    --- End diff --
   
    this is why travis failed, checkstyle violation


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Loading...