You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by omerhadari <gi...@git.apache.org> on 2017/09/24 19:34:28 UTC

[GitHub] nifi pull request #2171: NIFI-4392

GitHub user omerhadari opened a pull request:

    https://github.com/apache/nifi/pull/2171

    NIFI-4392

    Thank you for submitting a contribution to Apache NiFi.
    
    In order to streamline the review of the contribution we ask you
    to ensure the following steps have been taken:
    
    ### For all changes:
    - [v] Is there a JIRA ticket associated with this PR? Is it referenced 
         in the commit message?
    
    - [v] Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
    
    - [v] Has your PR been rebased against the latest commit within the target branch (typically master)?
    
    - [v] Is your initial contribution a single, squashed commit?
    
    ### For code changes:
    - [v] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
    - [v] Have you written or updated unit tests to verify your changes?
    - [v] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? 
    - [v] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
    - [v] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
    - [v] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
    
    Regarding NOTICE and LICENSE related changes, I think I don't need to touch them but I am not sure. I added dropwizard-metrics dependency (which is under ASF2 license), so verify me here please.
    
    Well, I hope I didn't go too far here and that I utilized controller services and reporting tasks the way they were meant to be used. I figured out after looking at `AmbariReportingTask` and `DataDogReportingTask` that most of the code is very (very) similar, as is quite noticeable when looking at their implementation. What is different between them is not the metrics that are being reported, but rather the report method itself. I chose to implement a single, generic reporting task that will rely on different implementations of a service which will provide it with a reporter. I figured that is rather similar to the DB connection pool service conceptually.
    If you think this is OK, I'd be more than happy to try and implement both the ambari and datadog reporting tasks the same way.
    
    Thanks :)

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

    $ git pull https://github.com/omerhadari/nifi nifi-4392

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

    https://github.com/apache/nifi/pull/2171.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 #2171
    
----
commit 9b574a18f92d5e8bf1d4fe9b77ae95422b72493b
Author: Omer Hadari <ha...@gmail.com>
Date:   2017-09-24T19:24:09Z

    NIFI-4392

----


---

[GitHub] nifi issue #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by omerhadari <gi...@git.apache.org>.
Github user omerhadari commented on the issue:

    https://github.com/apache/nifi/pull/2171
  
    Fixed, thank you for catching these!


---

[GitHub] nifi pull request #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by omerhadari <gi...@git.apache.org>.
Github user omerhadari commented on a diff in the pull request:

    https://github.com/apache/nifi/pull/2171#discussion_r143294672
  
    --- Diff: nifi-nar-bundles/nifi-metrics-reporting-bundle/nifi-metrics-reporting-task/src/main/java/org/apache/nifi/metrics/reporting/reporter/service/GraphiteMetricReporterService.java ---
    @@ -0,0 +1,147 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.nifi.metrics.reporting.reporter.service;
    +
    +import com.codahale.metrics.MetricRegistry;
    +import com.codahale.metrics.ScheduledReporter;
    +import com.codahale.metrics.graphite.Graphite;
    +import com.codahale.metrics.graphite.GraphiteReporter;
    +import com.codahale.metrics.graphite.GraphiteSender;
    +import org.apache.nifi.annotation.documentation.CapabilityDescription;
    +import org.apache.nifi.annotation.documentation.Tags;
    +import org.apache.nifi.annotation.lifecycle.OnDisabled;
    +import org.apache.nifi.annotation.lifecycle.OnEnabled;
    +import org.apache.nifi.components.PropertyDescriptor;
    +import org.apache.nifi.controller.AbstractControllerService;
    +import org.apache.nifi.controller.ConfigurationContext;
    +import org.apache.nifi.metrics.reporting.task.MetricsReportingTask;
    +import org.apache.nifi.processor.util.StandardValidators;
    +
    +import javax.net.SocketFactory;
    +import java.io.IOException;
    +import java.nio.charset.Charset;
    +import java.util.ArrayList;
    +import java.util.Collections;
    +import java.util.List;
    +
    +/**
    + * A controller service that provides metric reporters for graphite, can be used by {@link MetricsReportingTask}.
    + *
    + * @author Omer Hadari
    + */
    +@Tags({"metrics", "reporting", "graphite"})
    +@CapabilityDescription("A controller service that provides metric reporters for graphite. " +
    +        "Used by MetricsReportingTask.")
    +public class GraphiteMetricReporterService extends AbstractControllerService implements MetricReporterService {
    +
    +    /** Points to the hostname of the graphite listener. */
    +    public static final PropertyDescriptor HOST = new PropertyDescriptor.Builder()
    +            .name("host")
    +            .displayName("host")
    +            .description("The hostname of the carbon listener")
    +            .required(true)
    +            .addValidator(StandardValidators.URI_VALIDATOR)
    +            .build();
    +
    +    /** Points to the port on which the graphite server listens. */
    +    public static final PropertyDescriptor PORT = new PropertyDescriptor.Builder()
    +            .name("port")
    +            .displayName("port")
    +            .description("The port on which carbon listens")
    +            .required(true)
    +            .addValidator(StandardValidators.PORT_VALIDATOR)
    +            .build();
    +
    +    /** Points to the charset name that the graphite server expects. */
    +    public static final PropertyDescriptor CHARSET = new PropertyDescriptor.Builder()
    +            .name("charset")
    +            .displayName("charset")
    +            .description("The charset used by the graphite server")
    +            .required(true)
    +            .defaultValue("UTF-8")
    +            .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
    +            .build();
    +
    +    /** List of property descriptors used by the service. */
    +    private static final List<PropertyDescriptor> properties;
    +
    +    static {
    +        final List<PropertyDescriptor> props = new ArrayList<>();
    +        props.add(HOST);
    +        props.add(PORT);
    +        properties = Collections.unmodifiableList(props);
    +    }
    +
    +    /** Graphite sender, a connection to the server. */
    +    private GraphiteSender graphiteSender;
    +
    +    /**
    +     * Create the {@link #graphiteSender} according to configuration.
    +     *
    +     * @param context used to access properties.
    +     */
    +    @OnEnabled
    +    public void onEnabled(final ConfigurationContext context) {
    +        String host = context.getProperty(HOST).getValue();
    +        int port = context.getProperty(PORT).asInteger();
    +        Charset charset = Charset.forName(context.getProperty(CHARSET).getValue());
    +        graphiteSender = createSender(host, port, charset);
    +    }
    +
    +    /**
    +     * Close the graphite sender.
    +     *
    +     * @throws IOException if failed to close the connection.
    +     */
    +    @OnDisabled
    +    public void shutdown() throws IOException {
    +        try {
    +            graphiteSender.close();
    +        } finally {
    +            graphiteSender = null;
    +        }
    +    }
    +
    +    /**
    +     * Use the {@link #graphiteSender} in order to create a reporter.
    +     *
    +     * @param metricRegistry registry with the metrics to report.
    +     * @return a reporter instance.
    +     */
    +    @Override
    +    public ScheduledReporter createReporter(MetricRegistry metricRegistry) {
    +        return GraphiteReporter.forRegistry(metricRegistry).build(graphiteSender);
    +
    +    }
    +
    +    /**
    +     * Create a sender.
    +     *
    +     * @param host the hostname of the server to connect to.
    +     * @param port the port on which the server listens.
    +     * @param charset the charset in which the server expects logs.
    +     * @return The created sender.
    +     */
    +    protected GraphiteSender createSender(String host, int port, Charset charset) {
    +        return new Graphite(host, port, SocketFactory.getDefault(), charset);
    --- End diff --
    
    I am not a graphite expert, but as far as I know there is no native support for that - as graphite is meant to be used in a trusted (or secure) network. It might be possible to communicate with carbon using rabbitmq securely in some way, since it supports amqp and there is a rabbit mq client - but I feel that it is out of scope for this PR (correct me if I am wrong and I'll be happy to look into it).
    
    Also if someone can correct me on this and knows of a proper way to feed carbon with data in a secure fashion I'd be more than happy to stand corrected.


---

[GitHub] nifi pull request #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on a diff in the pull request:

    https://github.com/apache/nifi/pull/2171#discussion_r142977503
  
    --- Diff: nifi-nar-bundles/nifi-metrics-reporting-bundle/nifi-metrics-reporting-task/pom.xml ---
    @@ -0,0 +1,54 @@
    +<?xml version="1.0" encoding="UTF-8"?>
    +<!--
    +  Licensed to the Apache Software Foundation (ASF) under one or more
    +  contributor license agreements. See the NOTICE file distributed with
    +  this work for additional information regarding copyright ownership.
    +  The ASF licenses this file to You under the Apache License, Version 2.0
    +  (the "License"); you may not use this file except in compliance with
    +  the License. You may obtain a copy of the License at
    +  http://www.apache.org/licenses/LICENSE-2.0
    +  Unless required by applicable law or agreed to in writing, software
    +  distributed under the License is distributed on an "AS IS" BASIS,
    +  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    +  See the License for the specific language governing permissions and
    +  limitations under the License.
    +-->
    +<project xmlns="http://maven.apache.org/POM/4.0.0"
    +         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    +         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    +    <parent>
    +        <artifactId>nifi-metrics-reporting-bundle</artifactId>
    +        <groupId>org.apache.nifi</groupId>
    +        <version>1.4.0-SNAPSHOT</version>
    +    </parent>
    +    <modelVersion>4.0.0</modelVersion>
    +
    +    <artifactId>nifi-metrics-reporting-task</artifactId>
    +
    +    <dependencies>
    +        <dependency>
    +            <groupId>org.apache.nifi</groupId>
    +            <artifactId>nifi-utils</artifactId>
    +        </dependency>
    +        <dependency>
    +            <groupId>org.apache.nifi</groupId>
    +            <artifactId>nifi-metrics-reporter-service-api</artifactId>
    --- End diff --
    
    I think this one can be marked as 'provided' because at runtime it will be getting the API through the NAR dependency.


---

[GitHub] nifi issue #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on the issue:

    https://github.com/apache/nifi/pull/2171
  
    Update... You can ignore my comment about how to test it from earlier, I was able to get it to work now.
    
    This brings me to a new question, should we have a property for a prefix that would be appended to each metric name?
    
    Right now most of the metrics show up at the top-level and it isn't really clear that they came from NiFi, and if you were sending metrics from different NiFi instances to the same Graphite instance, you might want to separate them somehow.
    
    So maybe a property like "Metric Name Prefix" with a default value of "nifi"?


---

[GitHub] nifi pull request #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on a diff in the pull request:

    https://github.com/apache/nifi/pull/2171#discussion_r142978137
  
    --- Diff: nifi-nar-bundles/nifi-metrics-reporting-bundle/nifi-metrics-reporting-task/src/main/java/org/apache/nifi/metrics/reporting/reporter/service/GraphiteMetricReporterService.java ---
    @@ -0,0 +1,147 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.nifi.metrics.reporting.reporter.service;
    +
    +import com.codahale.metrics.MetricRegistry;
    +import com.codahale.metrics.ScheduledReporter;
    +import com.codahale.metrics.graphite.Graphite;
    +import com.codahale.metrics.graphite.GraphiteReporter;
    +import com.codahale.metrics.graphite.GraphiteSender;
    +import org.apache.nifi.annotation.documentation.CapabilityDescription;
    +import org.apache.nifi.annotation.documentation.Tags;
    +import org.apache.nifi.annotation.lifecycle.OnDisabled;
    +import org.apache.nifi.annotation.lifecycle.OnEnabled;
    +import org.apache.nifi.components.PropertyDescriptor;
    +import org.apache.nifi.controller.AbstractControllerService;
    +import org.apache.nifi.controller.ConfigurationContext;
    +import org.apache.nifi.metrics.reporting.task.MetricsReportingTask;
    +import org.apache.nifi.processor.util.StandardValidators;
    +
    +import javax.net.SocketFactory;
    +import java.io.IOException;
    +import java.nio.charset.Charset;
    +import java.util.ArrayList;
    +import java.util.Collections;
    +import java.util.List;
    +
    +/**
    + * A controller service that provides metric reporters for graphite, can be used by {@link MetricsReportingTask}.
    + *
    + * @author Omer Hadari
    + */
    +@Tags({"metrics", "reporting", "graphite"})
    +@CapabilityDescription("A controller service that provides metric reporters for graphite. " +
    +        "Used by MetricsReportingTask.")
    +public class GraphiteMetricReporterService extends AbstractControllerService implements MetricReporterService {
    +
    +    /** Points to the hostname of the graphite listener. */
    +    public static final PropertyDescriptor HOST = new PropertyDescriptor.Builder()
    +            .name("host")
    +            .displayName("host")
    +            .description("The hostname of the carbon listener")
    +            .required(true)
    +            .addValidator(StandardValidators.URI_VALIDATOR)
    +            .build();
    +
    +    /** Points to the port on which the graphite server listens. */
    +    public static final PropertyDescriptor PORT = new PropertyDescriptor.Builder()
    +            .name("port")
    +            .displayName("port")
    --- End diff --
    
    Should be "Port" for consistency in the UI


---

[GitHub] nifi pull request #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on a diff in the pull request:

    https://github.com/apache/nifi/pull/2171#discussion_r142978416
  
    --- Diff: nifi-nar-bundles/nifi-metrics-reporting-bundle/nifi-metrics-reporting-task/src/main/java/org/apache/nifi/metrics/reporting/reporter/service/GraphiteMetricReporterService.java ---
    @@ -0,0 +1,147 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.nifi.metrics.reporting.reporter.service;
    +
    +import com.codahale.metrics.MetricRegistry;
    +import com.codahale.metrics.ScheduledReporter;
    +import com.codahale.metrics.graphite.Graphite;
    +import com.codahale.metrics.graphite.GraphiteReporter;
    +import com.codahale.metrics.graphite.GraphiteSender;
    +import org.apache.nifi.annotation.documentation.CapabilityDescription;
    +import org.apache.nifi.annotation.documentation.Tags;
    +import org.apache.nifi.annotation.lifecycle.OnDisabled;
    +import org.apache.nifi.annotation.lifecycle.OnEnabled;
    +import org.apache.nifi.components.PropertyDescriptor;
    +import org.apache.nifi.controller.AbstractControllerService;
    +import org.apache.nifi.controller.ConfigurationContext;
    +import org.apache.nifi.metrics.reporting.task.MetricsReportingTask;
    +import org.apache.nifi.processor.util.StandardValidators;
    +
    +import javax.net.SocketFactory;
    +import java.io.IOException;
    +import java.nio.charset.Charset;
    +import java.util.ArrayList;
    +import java.util.Collections;
    +import java.util.List;
    +
    +/**
    + * A controller service that provides metric reporters for graphite, can be used by {@link MetricsReportingTask}.
    + *
    + * @author Omer Hadari
    + */
    +@Tags({"metrics", "reporting", "graphite"})
    +@CapabilityDescription("A controller service that provides metric reporters for graphite. " +
    +        "Used by MetricsReportingTask.")
    +public class GraphiteMetricReporterService extends AbstractControllerService implements MetricReporterService {
    +
    +    /** Points to the hostname of the graphite listener. */
    +    public static final PropertyDescriptor HOST = new PropertyDescriptor.Builder()
    +            .name("host")
    +            .displayName("host")
    +            .description("The hostname of the carbon listener")
    +            .required(true)
    +            .addValidator(StandardValidators.URI_VALIDATOR)
    +            .build();
    +
    +    /** Points to the port on which the graphite server listens. */
    +    public static final PropertyDescriptor PORT = new PropertyDescriptor.Builder()
    +            .name("port")
    +            .displayName("port")
    +            .description("The port on which carbon listens")
    +            .required(true)
    +            .addValidator(StandardValidators.PORT_VALIDATOR)
    +            .build();
    +
    +    /** Points to the charset name that the graphite server expects. */
    +    public static final PropertyDescriptor CHARSET = new PropertyDescriptor.Builder()
    +            .name("charset")
    +            .displayName("charset")
    +            .description("The charset used by the graphite server")
    +            .required(true)
    +            .defaultValue("UTF-8")
    +            .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
    +            .build();
    +
    +    /** List of property descriptors used by the service. */
    +    private static final List<PropertyDescriptor> properties;
    +
    +    static {
    +        final List<PropertyDescriptor> props = new ArrayList<>();
    +        props.add(HOST);
    +        props.add(PORT);
    --- End diff --
    
    CHARSET should be added to the list


---

[GitHub] nifi pull request #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on a diff in the pull request:

    https://github.com/apache/nifi/pull/2171#discussion_r143339228
  
    --- Diff: nifi-nar-bundles/nifi-metrics-reporting-bundle/nifi-metrics-reporting-task/src/main/java/org/apache/nifi/metrics/reporting/reporter/service/GraphiteMetricReporterService.java ---
    @@ -0,0 +1,180 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.nifi.metrics.reporting.reporter.service;
    +
    +import com.codahale.metrics.MetricRegistry;
    +import com.codahale.metrics.ScheduledReporter;
    +import com.codahale.metrics.graphite.Graphite;
    +import com.codahale.metrics.graphite.GraphiteReporter;
    +import com.codahale.metrics.graphite.GraphiteSender;
    +import org.apache.nifi.annotation.documentation.CapabilityDescription;
    +import org.apache.nifi.annotation.documentation.Tags;
    +import org.apache.nifi.annotation.lifecycle.OnDisabled;
    +import org.apache.nifi.annotation.lifecycle.OnEnabled;
    +import org.apache.nifi.components.PropertyDescriptor;
    +import org.apache.nifi.controller.AbstractControllerService;
    +import org.apache.nifi.controller.ConfigurationContext;
    +import org.apache.nifi.metrics.reporting.task.MetricsReportingTask;
    +import org.apache.nifi.processor.util.StandardValidators;
    +
    +import javax.net.SocketFactory;
    +import java.io.IOException;
    +import java.nio.charset.Charset;
    +import java.util.ArrayList;
    +import java.util.Collections;
    +import java.util.List;
    +
    +/**
    + * A controller service that provides metric reporters for graphite, can be used by {@link MetricsReportingTask}.
    + *
    + * @author Omer Hadari
    + */
    +@Tags({"metrics", "reporting", "graphite"})
    +@CapabilityDescription("A controller service that provides metric reporters for graphite. " +
    +        "Used by MetricsReportingTask.")
    +public class GraphiteMetricReporterService extends AbstractControllerService implements MetricReporterService {
    +
    +    /**
    +     * Points to the hostname of the graphite listener.
    +     */
    +    public static final PropertyDescriptor HOST = new PropertyDescriptor.Builder()
    +            .name("host")
    +            .displayName("Host")
    +            .description("The hostname of the carbon listener")
    +            .required(true)
    +            .addValidator(StandardValidators.URI_VALIDATOR)
    +            .expressionLanguageSupported(true)
    +            .build();
    +
    +    /**
    +     * Points to the port on which the graphite server listens.
    +     */
    +    public static final PropertyDescriptor PORT = new PropertyDescriptor.Builder()
    +            .name("port")
    +            .displayName("Port")
    +            .description("The port on which carbon listens")
    +            .required(true)
    +            .addValidator(StandardValidators.PORT_VALIDATOR)
    +            .expressionLanguageSupported(true)
    +            .build();
    +
    +    /**
    +     * Points to the charset name that the graphite server expects.
    +     */
    +    public static final PropertyDescriptor CHARSET = new PropertyDescriptor.Builder()
    +            .name("charset")
    +            .displayName("Charset")
    +            .description("The charset used by the graphite server")
    +            .required(true)
    +            .defaultValue("UTF-8")
    +            .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
    +            .expressionLanguageSupported(true)
    +            .build();
    +
    +    /**
    +     * Prefix for all metric names sent by reporters - for separation of NiFi stats in graphite.
    +     */
    +    protected static final PropertyDescriptor METRIC_NAME_PREFIX = new PropertyDescriptor.Builder()
    +            .name("metric name prefix")
    +            .displayName("Metric Name Prefix")
    +            .description("A prefix that will be used for all metric names sent by reporters provided by this service.")
    +            .required(true)
    +            .defaultValue("nifi")
    +            .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
    +            .expressionLanguageSupported(true)
    +            .build();
    +
    +    /**
    +     * List of property descriptors used by the service.
    +     */
    +    private static final List<PropertyDescriptor> properties;
    +
    +    static {
    +        final List<PropertyDescriptor> props = new ArrayList<>();
    +        props.add(HOST);
    +        props.add(PORT);
    +        props.add(CHARSET);
    +        properties = Collections.unmodifiableList(props);
    --- End diff --
    
    Need to add METRIC_NAME_PREFIX to this list


---

[GitHub] nifi pull request #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on a diff in the pull request:

    https://github.com/apache/nifi/pull/2171#discussion_r142978067
  
    --- Diff: nifi-nar-bundles/nifi-metrics-reporting-bundle/nifi-metrics-reporting-task/src/main/java/org/apache/nifi/metrics/reporting/reporter/service/GraphiteMetricReporterService.java ---
    @@ -0,0 +1,147 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.nifi.metrics.reporting.reporter.service;
    +
    +import com.codahale.metrics.MetricRegistry;
    +import com.codahale.metrics.ScheduledReporter;
    +import com.codahale.metrics.graphite.Graphite;
    +import com.codahale.metrics.graphite.GraphiteReporter;
    +import com.codahale.metrics.graphite.GraphiteSender;
    +import org.apache.nifi.annotation.documentation.CapabilityDescription;
    +import org.apache.nifi.annotation.documentation.Tags;
    +import org.apache.nifi.annotation.lifecycle.OnDisabled;
    +import org.apache.nifi.annotation.lifecycle.OnEnabled;
    +import org.apache.nifi.components.PropertyDescriptor;
    +import org.apache.nifi.controller.AbstractControllerService;
    +import org.apache.nifi.controller.ConfigurationContext;
    +import org.apache.nifi.metrics.reporting.task.MetricsReportingTask;
    +import org.apache.nifi.processor.util.StandardValidators;
    +
    +import javax.net.SocketFactory;
    +import java.io.IOException;
    +import java.nio.charset.Charset;
    +import java.util.ArrayList;
    +import java.util.Collections;
    +import java.util.List;
    +
    +/**
    + * A controller service that provides metric reporters for graphite, can be used by {@link MetricsReportingTask}.
    + *
    + * @author Omer Hadari
    + */
    +@Tags({"metrics", "reporting", "graphite"})
    +@CapabilityDescription("A controller service that provides metric reporters for graphite. " +
    +        "Used by MetricsReportingTask.")
    +public class GraphiteMetricReporterService extends AbstractControllerService implements MetricReporterService {
    +
    +    /** Points to the hostname of the graphite listener. */
    +    public static final PropertyDescriptor HOST = new PropertyDescriptor.Builder()
    +            .name("host")
    +            .displayName("host")
    --- End diff --
    
    Should be "Host" for consistency in the UI


---

[GitHub] nifi pull request #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on a diff in the pull request:

    https://github.com/apache/nifi/pull/2171#discussion_r143339299
  
    --- Diff: nifi-nar-bundles/nifi-metrics-reporting-bundle/nifi-metrics-reporter-service-api-nar/src/main/resources/NOTICE ---
    @@ -0,0 +1,25 @@
    +nifi-metrics-reporter-service-api-nar
    --- End diff --
    
    Looks good, just need to be moved under a META-INF directory so it would be under src/main/resources/META-INF


---

[GitHub] nifi pull request #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/nifi/pull/2171


---

[GitHub] nifi issue #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by omerhadari <gi...@git.apache.org>.
Github user omerhadari commented on the issue:

    https://github.com/apache/nifi/pull/2171
  
    Done :)
    I noticed that the ambari reporting task allows reporting of group-specific metrics, so I added that as an option to the reporting task as well.
    Also I wasn't sure if you want me to squash the commits, let me know if you do.


---

[GitHub] nifi pull request #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on a diff in the pull request:

    https://github.com/apache/nifi/pull/2171#discussion_r142978942
  
    --- Diff: nifi-nar-bundles/nifi-metrics-reporting-bundle/nifi-metrics-reporting-task/src/main/java/org/apache/nifi/metrics/reporting/reporter/service/GraphiteMetricReporterService.java ---
    @@ -0,0 +1,147 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.nifi.metrics.reporting.reporter.service;
    +
    +import com.codahale.metrics.MetricRegistry;
    +import com.codahale.metrics.ScheduledReporter;
    +import com.codahale.metrics.graphite.Graphite;
    +import com.codahale.metrics.graphite.GraphiteReporter;
    +import com.codahale.metrics.graphite.GraphiteSender;
    +import org.apache.nifi.annotation.documentation.CapabilityDescription;
    +import org.apache.nifi.annotation.documentation.Tags;
    +import org.apache.nifi.annotation.lifecycle.OnDisabled;
    +import org.apache.nifi.annotation.lifecycle.OnEnabled;
    +import org.apache.nifi.components.PropertyDescriptor;
    +import org.apache.nifi.controller.AbstractControllerService;
    +import org.apache.nifi.controller.ConfigurationContext;
    +import org.apache.nifi.metrics.reporting.task.MetricsReportingTask;
    +import org.apache.nifi.processor.util.StandardValidators;
    +
    +import javax.net.SocketFactory;
    +import java.io.IOException;
    +import java.nio.charset.Charset;
    +import java.util.ArrayList;
    +import java.util.Collections;
    +import java.util.List;
    +
    +/**
    + * A controller service that provides metric reporters for graphite, can be used by {@link MetricsReportingTask}.
    + *
    + * @author Omer Hadari
    + */
    +@Tags({"metrics", "reporting", "graphite"})
    +@CapabilityDescription("A controller service that provides metric reporters for graphite. " +
    +        "Used by MetricsReportingTask.")
    +public class GraphiteMetricReporterService extends AbstractControllerService implements MetricReporterService {
    +
    +    /** Points to the hostname of the graphite listener. */
    +    public static final PropertyDescriptor HOST = new PropertyDescriptor.Builder()
    +            .name("host")
    +            .displayName("host")
    +            .description("The hostname of the carbon listener")
    +            .required(true)
    +            .addValidator(StandardValidators.URI_VALIDATOR)
    +            .build();
    +
    +    /** Points to the port on which the graphite server listens. */
    +    public static final PropertyDescriptor PORT = new PropertyDescriptor.Builder()
    +            .name("port")
    +            .displayName("port")
    +            .description("The port on which carbon listens")
    +            .required(true)
    +            .addValidator(StandardValidators.PORT_VALIDATOR)
    +            .build();
    +
    +    /** Points to the charset name that the graphite server expects. */
    +    public static final PropertyDescriptor CHARSET = new PropertyDescriptor.Builder()
    +            .name("charset")
    +            .displayName("charset")
    +            .description("The charset used by the graphite server")
    +            .required(true)
    +            .defaultValue("UTF-8")
    +            .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
    +            .build();
    +
    +    /** List of property descriptors used by the service. */
    +    private static final List<PropertyDescriptor> properties;
    +
    +    static {
    +        final List<PropertyDescriptor> props = new ArrayList<>();
    +        props.add(HOST);
    +        props.add(PORT);
    +        properties = Collections.unmodifiableList(props);
    +    }
    +
    +    /** Graphite sender, a connection to the server. */
    +    private GraphiteSender graphiteSender;
    +
    +    /**
    +     * Create the {@link #graphiteSender} according to configuration.
    +     *
    +     * @param context used to access properties.
    +     */
    +    @OnEnabled
    +    public void onEnabled(final ConfigurationContext context) {
    +        String host = context.getProperty(HOST).getValue();
    +        int port = context.getProperty(PORT).asInteger();
    --- End diff --
    
    It would be good to support expression language for host and port so that people can easily use variables to change environments. 
    
    You can just add `supportsExpressionLanguage(true)` on the property descriptors and then here when you get the values do `context.getProperty(HOST).evaluateAttributeExpressions().getValue()`


---

[GitHub] nifi pull request #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on a diff in the pull request:

    https://github.com/apache/nifi/pull/2171#discussion_r142979354
  
    --- Diff: nifi-nar-bundles/nifi-metrics-reporting-bundle/nifi-metrics-reporting-task/src/main/java/org/apache/nifi/metrics/reporting/task/MetricsReportingTask.java ---
    @@ -0,0 +1,125 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.nifi.metrics.reporting.task;
    +
    +import com.codahale.metrics.MetricRegistry;
    +import com.codahale.metrics.ScheduledReporter;
    +import com.codahale.metrics.jvm.MemoryUsageGaugeSet;
    +import org.apache.nifi.annotation.documentation.CapabilityDescription;
    +import org.apache.nifi.annotation.documentation.Tags;
    +import org.apache.nifi.annotation.lifecycle.OnScheduled;
    +import org.apache.nifi.components.PropertyDescriptor;
    +import org.apache.nifi.controller.ConfigurationContext;
    +import org.apache.nifi.controller.status.ProcessGroupStatus;
    +import org.apache.nifi.metrics.FlowMetricSet;
    +import org.apache.nifi.metrics.reporting.reporter.service.MetricReporterService;
    +import org.apache.nifi.reporting.AbstractReportingTask;
    +import org.apache.nifi.reporting.ReportingContext;
    +import org.apache.nifi.reporting.ReportingInitializationContext;
    +
    +import java.util.ArrayList;
    +import java.util.Collections;
    +import java.util.List;
    +import java.util.concurrent.atomic.AtomicReference;
    +
    +/**
    + * A reporting task for NiFi instance and JVM related metrics.
    + *
    + * This task reports metrics to services according to a provided {@link ScheduledReporter}, reached by using a
    + * {@link MetricReporterService}. In order to report to different clients, simply use different implementations of
    + * the controller service.
    + *
    + * @see MetricReporterService
    + * @author Omer Hadari
    + */
    +@Tags({"metrics", "reporting"})
    +@CapabilityDescription("This reporting task reports a set of metrics regarding the JVM and the NiFi instance" +
    +        "to a reporter. The reporter is provided by a MetricReporterService.")
    +public class MetricsReportingTask extends AbstractReportingTask {
    +
    +    /**
    +     * Points to the service which provides {@link ScheduledReporter} instances.
    +     */
    +    protected static final PropertyDescriptor REPORTER_SERVICE = new PropertyDescriptor.Builder()
    +            .name("metric reporter service")
    +            .displayName("metric reporter service")
    --- End diff --
    
    Should be "Metric Reporter Service" for consistency in the UI


---

[GitHub] nifi pull request #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on a diff in the pull request:

    https://github.com/apache/nifi/pull/2171#discussion_r143339275
  
    --- Diff: nifi-nar-bundles/nifi-metrics-reporting-bundle/nifi-metrics-reporting-task/src/main/java/org/apache/nifi/metrics/reporting/reporter/service/GraphiteMetricReporterService.java ---
    @@ -0,0 +1,180 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.nifi.metrics.reporting.reporter.service;
    +
    +import com.codahale.metrics.MetricRegistry;
    +import com.codahale.metrics.ScheduledReporter;
    +import com.codahale.metrics.graphite.Graphite;
    +import com.codahale.metrics.graphite.GraphiteReporter;
    +import com.codahale.metrics.graphite.GraphiteSender;
    +import org.apache.nifi.annotation.documentation.CapabilityDescription;
    +import org.apache.nifi.annotation.documentation.Tags;
    +import org.apache.nifi.annotation.lifecycle.OnDisabled;
    +import org.apache.nifi.annotation.lifecycle.OnEnabled;
    +import org.apache.nifi.components.PropertyDescriptor;
    +import org.apache.nifi.controller.AbstractControllerService;
    +import org.apache.nifi.controller.ConfigurationContext;
    +import org.apache.nifi.metrics.reporting.task.MetricsReportingTask;
    +import org.apache.nifi.processor.util.StandardValidators;
    +
    +import javax.net.SocketFactory;
    +import java.io.IOException;
    +import java.nio.charset.Charset;
    +import java.util.ArrayList;
    +import java.util.Collections;
    +import java.util.List;
    +
    +/**
    + * A controller service that provides metric reporters for graphite, can be used by {@link MetricsReportingTask}.
    + *
    + * @author Omer Hadari
    + */
    +@Tags({"metrics", "reporting", "graphite"})
    +@CapabilityDescription("A controller service that provides metric reporters for graphite. " +
    +        "Used by MetricsReportingTask.")
    +public class GraphiteMetricReporterService extends AbstractControllerService implements MetricReporterService {
    +
    +    /**
    +     * Points to the hostname of the graphite listener.
    +     */
    +    public static final PropertyDescriptor HOST = new PropertyDescriptor.Builder()
    +            .name("host")
    +            .displayName("Host")
    +            .description("The hostname of the carbon listener")
    +            .required(true)
    +            .addValidator(StandardValidators.URI_VALIDATOR)
    +            .expressionLanguageSupported(true)
    +            .build();
    +
    +    /**
    +     * Points to the port on which the graphite server listens.
    +     */
    +    public static final PropertyDescriptor PORT = new PropertyDescriptor.Builder()
    +            .name("port")
    +            .displayName("Port")
    +            .description("The port on which carbon listens")
    +            .required(true)
    +            .addValidator(StandardValidators.PORT_VALIDATOR)
    +            .expressionLanguageSupported(true)
    +            .build();
    +
    +    /**
    +     * Points to the charset name that the graphite server expects.
    +     */
    +    public static final PropertyDescriptor CHARSET = new PropertyDescriptor.Builder()
    +            .name("charset")
    +            .displayName("Charset")
    +            .description("The charset used by the graphite server")
    +            .required(true)
    +            .defaultValue("UTF-8")
    +            .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
    +            .expressionLanguageSupported(true)
    +            .build();
    +
    +    /**
    +     * Prefix for all metric names sent by reporters - for separation of NiFi stats in graphite.
    +     */
    +    protected static final PropertyDescriptor METRIC_NAME_PREFIX = new PropertyDescriptor.Builder()
    +            .name("metric name prefix")
    +            .displayName("Metric Name Prefix")
    +            .description("A prefix that will be used for all metric names sent by reporters provided by this service.")
    +            .required(true)
    +            .defaultValue("nifi")
    +            .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
    +            .expressionLanguageSupported(true)
    +            .build();
    +
    +    /**
    +     * List of property descriptors used by the service.
    +     */
    +    private static final List<PropertyDescriptor> properties;
    +
    +    static {
    +        final List<PropertyDescriptor> props = new ArrayList<>();
    +        props.add(HOST);
    +        props.add(PORT);
    +        props.add(CHARSET);
    +        properties = Collections.unmodifiableList(props);
    +    }
    +
    +    /**
    +     * Graphite sender, a connection to the server.
    +     */
    +    private GraphiteSender graphiteSender;
    +
    +    /**
    +     * The configured {@link #METRIC_NAME_PREFIX} value.
    +     */
    +    private String metricNamePrefix;
    +
    +    /**
    +     * Create the {@link #graphiteSender} according to configuration.
    +     *
    +     * @param context used to access properties.
    +     */
    +    @OnEnabled
    +    public void onEnabled(final ConfigurationContext context) {
    +        String host = context.getProperty(HOST).evaluateAttributeExpressions().getValue();
    +        int port = context.getProperty(PORT).evaluateAttributeExpressions().asInteger();
    +        Charset charset = Charset.forName(context.getProperty(CHARSET).getValue());
    --- End diff --
    
    If the property descriptor for charset say it supports expression language then we need to call evaluateAttributeExpressions() here, or don't support el for charset, either way is fine


---

[GitHub] nifi pull request #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on a diff in the pull request:

    https://github.com/apache/nifi/pull/2171#discussion_r143339295
  
    --- Diff: nifi-nar-bundles/nifi-metrics-reporting-bundle/nifi-metrics-reporting-nar/src/main/resources/NOTICE ---
    @@ -0,0 +1,25 @@
    +nifi-metrics-reporting-nar
    --- End diff --
    
    Looks good, just need to be moved under a META-INF directory so it would be under src/main/resources/META-INF


---

[GitHub] nifi issue #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:

    https://github.com/apache/nifi/pull/2171
  
    @omerhadari usually the reviewer will squash the commits. It’s nice if the initial PR is squashed if it’s a single unit of work, but with evolutionary interaction like this, it’s not a big deal. As long as the code is rebased against current master, it’s a pretty easy process. 


---

[GitHub] nifi issue #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by omerhadari <gi...@git.apache.org>.
Github user omerhadari commented on the issue:

    https://github.com/apache/nifi/pull/2171
  
    Thanks for the review, I am on it


---

[GitHub] nifi pull request #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on a diff in the pull request:

    https://github.com/apache/nifi/pull/2171#discussion_r142978216
  
    --- Diff: nifi-nar-bundles/nifi-metrics-reporting-bundle/nifi-metrics-reporting-task/src/main/java/org/apache/nifi/metrics/reporting/reporter/service/GraphiteMetricReporterService.java ---
    @@ -0,0 +1,147 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.nifi.metrics.reporting.reporter.service;
    +
    +import com.codahale.metrics.MetricRegistry;
    +import com.codahale.metrics.ScheduledReporter;
    +import com.codahale.metrics.graphite.Graphite;
    +import com.codahale.metrics.graphite.GraphiteReporter;
    +import com.codahale.metrics.graphite.GraphiteSender;
    +import org.apache.nifi.annotation.documentation.CapabilityDescription;
    +import org.apache.nifi.annotation.documentation.Tags;
    +import org.apache.nifi.annotation.lifecycle.OnDisabled;
    +import org.apache.nifi.annotation.lifecycle.OnEnabled;
    +import org.apache.nifi.components.PropertyDescriptor;
    +import org.apache.nifi.controller.AbstractControllerService;
    +import org.apache.nifi.controller.ConfigurationContext;
    +import org.apache.nifi.metrics.reporting.task.MetricsReportingTask;
    +import org.apache.nifi.processor.util.StandardValidators;
    +
    +import javax.net.SocketFactory;
    +import java.io.IOException;
    +import java.nio.charset.Charset;
    +import java.util.ArrayList;
    +import java.util.Collections;
    +import java.util.List;
    +
    +/**
    + * A controller service that provides metric reporters for graphite, can be used by {@link MetricsReportingTask}.
    + *
    + * @author Omer Hadari
    + */
    +@Tags({"metrics", "reporting", "graphite"})
    +@CapabilityDescription("A controller service that provides metric reporters for graphite. " +
    +        "Used by MetricsReportingTask.")
    +public class GraphiteMetricReporterService extends AbstractControllerService implements MetricReporterService {
    +
    +    /** Points to the hostname of the graphite listener. */
    +    public static final PropertyDescriptor HOST = new PropertyDescriptor.Builder()
    +            .name("host")
    +            .displayName("host")
    +            .description("The hostname of the carbon listener")
    +            .required(true)
    +            .addValidator(StandardValidators.URI_VALIDATOR)
    +            .build();
    +
    +    /** Points to the port on which the graphite server listens. */
    +    public static final PropertyDescriptor PORT = new PropertyDescriptor.Builder()
    +            .name("port")
    +            .displayName("port")
    +            .description("The port on which carbon listens")
    +            .required(true)
    +            .addValidator(StandardValidators.PORT_VALIDATOR)
    +            .build();
    +
    +    /** Points to the charset name that the graphite server expects. */
    +    public static final PropertyDescriptor CHARSET = new PropertyDescriptor.Builder()
    +            .name("charset")
    +            .displayName("charset")
    --- End diff --
    
    Should be "Charset" for consistency in the UI


---

[GitHub] nifi pull request #2171: NIFI-4392 - Add metric reporting task for Graphite

Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on a diff in the pull request:

    https://github.com/apache/nifi/pull/2171#discussion_r142979242
  
    --- Diff: nifi-nar-bundles/nifi-metrics-reporting-bundle/nifi-metrics-reporting-task/src/main/java/org/apache/nifi/metrics/reporting/reporter/service/GraphiteMetricReporterService.java ---
    @@ -0,0 +1,147 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.nifi.metrics.reporting.reporter.service;
    +
    +import com.codahale.metrics.MetricRegistry;
    +import com.codahale.metrics.ScheduledReporter;
    +import com.codahale.metrics.graphite.Graphite;
    +import com.codahale.metrics.graphite.GraphiteReporter;
    +import com.codahale.metrics.graphite.GraphiteSender;
    +import org.apache.nifi.annotation.documentation.CapabilityDescription;
    +import org.apache.nifi.annotation.documentation.Tags;
    +import org.apache.nifi.annotation.lifecycle.OnDisabled;
    +import org.apache.nifi.annotation.lifecycle.OnEnabled;
    +import org.apache.nifi.components.PropertyDescriptor;
    +import org.apache.nifi.controller.AbstractControllerService;
    +import org.apache.nifi.controller.ConfigurationContext;
    +import org.apache.nifi.metrics.reporting.task.MetricsReportingTask;
    +import org.apache.nifi.processor.util.StandardValidators;
    +
    +import javax.net.SocketFactory;
    +import java.io.IOException;
    +import java.nio.charset.Charset;
    +import java.util.ArrayList;
    +import java.util.Collections;
    +import java.util.List;
    +
    +/**
    + * A controller service that provides metric reporters for graphite, can be used by {@link MetricsReportingTask}.
    + *
    + * @author Omer Hadari
    + */
    +@Tags({"metrics", "reporting", "graphite"})
    +@CapabilityDescription("A controller service that provides metric reporters for graphite. " +
    +        "Used by MetricsReportingTask.")
    +public class GraphiteMetricReporterService extends AbstractControllerService implements MetricReporterService {
    +
    +    /** Points to the hostname of the graphite listener. */
    +    public static final PropertyDescriptor HOST = new PropertyDescriptor.Builder()
    +            .name("host")
    +            .displayName("host")
    +            .description("The hostname of the carbon listener")
    +            .required(true)
    +            .addValidator(StandardValidators.URI_VALIDATOR)
    +            .build();
    +
    +    /** Points to the port on which the graphite server listens. */
    +    public static final PropertyDescriptor PORT = new PropertyDescriptor.Builder()
    +            .name("port")
    +            .displayName("port")
    +            .description("The port on which carbon listens")
    +            .required(true)
    +            .addValidator(StandardValidators.PORT_VALIDATOR)
    +            .build();
    +
    +    /** Points to the charset name that the graphite server expects. */
    +    public static final PropertyDescriptor CHARSET = new PropertyDescriptor.Builder()
    +            .name("charset")
    +            .displayName("charset")
    +            .description("The charset used by the graphite server")
    +            .required(true)
    +            .defaultValue("UTF-8")
    +            .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
    +            .build();
    +
    +    /** List of property descriptors used by the service. */
    +    private static final List<PropertyDescriptor> properties;
    +
    +    static {
    +        final List<PropertyDescriptor> props = new ArrayList<>();
    +        props.add(HOST);
    +        props.add(PORT);
    +        properties = Collections.unmodifiableList(props);
    +    }
    +
    +    /** Graphite sender, a connection to the server. */
    +    private GraphiteSender graphiteSender;
    +
    +    /**
    +     * Create the {@link #graphiteSender} according to configuration.
    +     *
    +     * @param context used to access properties.
    +     */
    +    @OnEnabled
    +    public void onEnabled(final ConfigurationContext context) {
    +        String host = context.getProperty(HOST).getValue();
    +        int port = context.getProperty(PORT).asInteger();
    +        Charset charset = Charset.forName(context.getProperty(CHARSET).getValue());
    +        graphiteSender = createSender(host, port, charset);
    +    }
    +
    +    /**
    +     * Close the graphite sender.
    +     *
    +     * @throws IOException if failed to close the connection.
    +     */
    +    @OnDisabled
    +    public void shutdown() throws IOException {
    +        try {
    +            graphiteSender.close();
    +        } finally {
    +            graphiteSender = null;
    +        }
    +    }
    +
    +    /**
    +     * Use the {@link #graphiteSender} in order to create a reporter.
    +     *
    +     * @param metricRegistry registry with the metrics to report.
    +     * @return a reporter instance.
    +     */
    +    @Override
    +    public ScheduledReporter createReporter(MetricRegistry metricRegistry) {
    +        return GraphiteReporter.forRegistry(metricRegistry).build(graphiteSender);
    +
    +    }
    +
    +    /**
    +     * Create a sender.
    +     *
    +     * @param host the hostname of the server to connect to.
    +     * @param port the port on which the server listens.
    +     * @param charset the charset in which the server expects logs.
    +     * @return The created sender.
    +     */
    +    protected GraphiteSender createSender(String host, int port, Charset charset) {
    +        return new Graphite(host, port, SocketFactory.getDefault(), charset);
    --- End diff --
    
    Is there ever a case where the metrics would be sent over an SSL/TLS connection?


---