Prometheus Sink draft code for issue #1744. #3103

mallikagogoi7 · 2023-08-02T14:26:39Z

Description

Prometheus Sink draft code for issue #1744.

Issues Resolved

Check List

New functionality includes testing.
New functionality has been documented.
- New functionality has javadoc added
Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: mallikagogoi7 <[email protected]>

…raft

Signed-off-by: mallikagogoi7 <[email protected]>

…project#1744. Signed-off-by: mallikagogoi7 <[email protected]>

…raft

dinujoh

General comments:

Use final on local and method variables.
Add metrics for success and failure scenarios.

dinujoh · 2023-08-11T15:38:02Z

data-prepper-plugins/prometheus-sink/README.md

+
+- `url` The http/https endpoint url which can bee backed by prometheus.
+
+- `encoding`  Default is snappy


Do we support other encoding and content-type ?

@dinujoh Thanks for your review. No, currently only snappy is required to support Prometheus.

Since we don't support any other configuration, this configuration parameter is not adding any value unless we have a followup plan to support another encoding. Can you followup and check what is the plan ?

dinujoh · 2023-08-11T15:53:36Z

...ionTest/java/org/opensearch/dataprepper/plugins/sink/prometheus/PrometheusSinkServiceIT.java

+    @BeforeEach
+    void setUp() throws JsonProcessingException{
+        this.urlString = System.getProperty("tests.prometheus.sink.http.endpoint");
+        String[] values = { urlString,"unauthenticated","ap-south-1","arn:aws:iam::524239988944:role/app-test" };


the role and region needs to be configurable.

@dinujoh Thanks for your review. It should be configurable and should be read from system properties. Will update the code accordingly.

dinujoh · 2023-08-14T15:12:27Z

...a/org/opensearch/dataprepper/plugins/sink/prometheus/AwsRequestSigningApacheInterceptor.java

+ * An {@link HttpRequestInterceptor} that signs requests using any AWS {@link Signer}
+ * and {@link AwsCredentialsProvider}.
+ */
+public final class AwsRequestSigningApacheInterceptor implements HttpRequestInterceptor {


This class code is duplicated. We need to move this to common plugin module.

This can be moved to aws-plugin-api

@dinujoh Thanks for your review. Will update the code accordingly.

dinujoh · 2023-08-14T15:24:06Z

...g/opensearch/dataprepper/plugins/sink/prometheus/certificate/CertificateProviderFactory.java

+ * This class consist logic for downloading the SSL certificates from S3/ACM/Local file.
+ *
+ */
+public class CertificateProviderFactory {


This class is duplicated as well. Need to move this to common module.

Move this to common certificate module. This class would need refactoring to remove direct dependency on PrometheusSinkConfiguration and set the required parameters in the constructor or create common object for them.

@dinujoh Thanks for your review. Will update the code accordingly.

dinujoh · 2023-08-14T15:47:07Z

...n/java/org/opensearch/dataprepper/plugins/sink/prometheus/service/PrometheusSinkService.java

+                    if (event instanceof JacksonGauge) {
+                        JacksonGauge jacksonGauge = (JacksonGauge) event;
+                        message = buildRemoteWriteRequest(jacksonGauge.getTime(),
+                                jacksonGauge.getStartTime(), jacksonGauge.getValue(), jacksonGauge.getAttributes(),jacksonGauge.getName());
+                    } else if (event instanceof JacksonSum) {
+                        JacksonSum jacksonSum = (JacksonSum) event;
+                        message = buildRemoteWriteRequest(jacksonSum.getTime(),
+                                jacksonSum.getStartTime(), jacksonSum.getValue(), jacksonSum.getAttributes(), jacksonSum.getName());
+                    } else if (event instanceof JacksonSummary) {
+                        JacksonSummary jacksonSummary = (JacksonSummary) event;
+                        message = buildRemoteWriteRequest(jacksonSummary.getTime(),
+                                jacksonSummary.getStartTime(), jacksonSummary.getSum(), jacksonSummary.getAttributes(), jacksonSummary.getName());
+                    } else if (event instanceof JacksonHistogram) {
+                        JacksonHistogram jacksonHistogram = (JacksonHistogram) event;
+                        message = buildRemoteWriteRequest(jacksonHistogram.getTime(),
+                                jacksonHistogram.getStartTime(), jacksonHistogram.getSum(), jacksonHistogram.getAttributes(), jacksonHistogram.getName());
+                    } else if (event instanceof JacksonExponentialHistogram) {
+                        JacksonExponentialHistogram jacksonExpHistogram = (JacksonExponentialHistogram) event;
+                        message = buildRemoteWriteRequest(jacksonExpHistogram.getTime(),
+                                jacksonExpHistogram.getStartTime(), jacksonExpHistogram.getSum(), jacksonExpHistogram.getAttributes(), jacksonExpHistogram.getName());
+                    } else {


I see getSum method is not part of interface. Can we avoid this multiple if else statement and use reflection to call the getSum method ?

@dinujoh Thanks for your review. Will update the code accordingly.

@dinujoh Can we please elaborate on that?

dinujoh · 2023-08-14T15:49:16Z

...n/java/org/opensearch/dataprepper/plugins/sink/prometheus/service/PrometheusSinkService.java

+                try {
+                    failedHttpEndPointResponses = pushToEndPoint(bytes);
+                } catch (IOException e) {
+                    LOG.info("Error while pushing to the end point");


We should add metric when this failure occurs.

@dinujoh Thanks for your review. Will update the code accordingly.

dinujoh · 2023-08-14T15:49:41Z

...n/java/org/opensearch/dataprepper/plugins/sink/prometheus/service/PrometheusSinkService.java

+                if (failedHttpEndPointResponses != null) {
+                    logFailedData(failedHttpEndPointResponses);
+                    releaseEventHandles(Boolean.FALSE);
+                } else {


This is not true when exception occurs at line 166.

@omkarmmore95 and @mallikagogoi7 Could you please have a look and clarify?

@dinujoh The code has been updated accordingly

dinujoh · 2023-08-14T15:52:13Z

...n/java/org/opensearch/dataprepper/plugins/sink/prometheus/service/PrometheusSinkService.java

+        final ClassicRequestBuilder classicHttpRequestBuilder =
+                httpAuthOptions.get(prometheusSinkConfiguration.getUrl()).getClassicHttpRequestBuilder();
+
+        final byte[] compressedBufferData = Snappy.compress(data);


Will the data be always Snappy compressed ? Should this be configurable ?

@dinujoh Thanks for your review. Yes, only snappy compression is required to send data to Prometheus.

dinujoh · 2023-08-14T15:53:01Z

...n/java/org/opensearch/dataprepper/plugins/sink/prometheus/service/PrometheusSinkService.java

+        classicHttpRequestBuilder.addHeader("X-Prometheus-Remote-Write-Version", prometheusSinkConfiguration.getRemoteWriteVersion());
+
+        try {
+            if(AuthTypeOptions.BEARER_TOKEN.equals(prometheusSinkConfiguration.getAuthType()))


How about other Auth support ?

@omkarmmore95 and @mallikagogoi7 Could you please clarify?

@dinujoh , we have unauthenticated, http_basic, and bearer_token auth just like Http Sink plugin

dinujoh · 2023-08-14T16:51:38Z

data-prepper-plugins/prometheus-sink/README.md

+
+- `scope` (Optional) : This scope limit an application's access to a user's account.
+
+- `aws_sigv4`: A boolean flag to sign the HTTP request with AWS credentials. Default to `false`.


We should use aws: property for sigv4 and not use aws_.

@dinujoh Thanks for your review. We are following the other plugins (opensearch, httpsink etc.) key naming conventions to define the aws_sigv4 property.

The new convention is to move away from aws_ for the new sources and sinks.. We should only use aws: for new sources and sinks.

…raft

Signed-off-by: mallikagogoi7 <[email protected]>

mallikagogoi7 · 2023-08-18T10:03:55Z

General comments:

Use final on local and method variables.

Add metrics for success and failure scenarios.

@dinujoh Above comments has been addressed

Signed-off-by: mallikagogoi7 <[email protected]>

…raft

Signed-off-by: mallikagogoi7 <[email protected]>

dinujoh · 2023-08-23T14:49:07Z

data-prepper-plugins/prometheus-sink/README.md

+
+- `scope` (Optional) : This scope limit an application's access to a user's account.
+
+- `aws_sigv4`: A boolean flag to sign the HTTP request with AWS credentials. Default to `false`.


The new convention is to move away from aws_ for the new sources and sinks.. We should only use aws: for new sources and sinks.

dinujoh · 2023-08-23T14:50:20Z

data-prepper-plugins/prometheus-sink/README.md

+
+- `url` The http/https endpoint url which can bee backed by prometheus.
+
+- `encoding`  Default is snappy


Since we don't support any other configuration, this configuration parameter is not adding any value unless we have a followup plan to support another encoding. Can you followup and check what is the plan ?

dinujoh · 2023-08-23T14:52:12Z

...ionTest/java/org/opensearch/dataprepper/plugins/sink/prometheus/PrometheusSinkServiceIT.java

+            "        url: {0}\n" +
+                    "        http_method: POST\n" +
+                    "        auth_type: {1}\n" +
+                    "        ssl: false\n";


How about other configurations for Integration test ?

@dino, Due to less time for this plugin, integration test was not in scope

dinujoh · 2023-08-23T14:54:30Z

...n/java/org/opensearch/dataprepper/plugins/sink/prometheus/FailedHttpResponseInterceptor.java

-       //TODO: implementation
+    public void process(final HttpResponse response, final EntityDetails entity, final HttpContext context) throws IOException {
+        if (response.getCode() != STATUS_CODE_200) {
+            throw new IOException(String.format("url:  %s , status code: %s", url,response.getCode()));


Why is this IOException ?

@dino, Thanks for review,For failed response we are throwing IOException, and on IOException, we are sending Failed data to DLQ

dinujoh · 2023-08-23T14:56:01Z

...rc/main/java/org/opensearch/dataprepper/plugins/sink/prometheus/OAuthAccessTokenManager.java

+        }catch (Exception e) {
+            LOG.info("Exception : "+ e.getMessage() );
+        }
+        return bearerTokenOptions.getAccessToken();


if the exception is thrown in line 37, what would be this return value ? Will this cause NPE ?

@dinujoh , Thanks for the review, Resolved

dinujoh · 2023-08-23T14:58:28Z

...ensearch/dataprepper/plugins/sink/prometheus/certificate/HttpClientSSLConnectionManager.java

+            Certificate trustedCa;
+            trustedCa = factory.generateCertificate(certificate);


this can be single line

@dinujoh , Thanks for the review, Resolved

dinujoh · 2023-08-24T13:57:43Z

...ink/src/main/java/org/opensearch/dataprepper/plugins/sink/prometheus/dlq/DlqPushHandler.java

+                StandardOpenOption.CREATE, StandardOpenOption.APPEND)) {
+            dlqFileWriter.write(objectWriter.writeValueAsString(failedData)+"\n");
+        } catch (IOException e) {
+            LOG.error("Exception while writing failed data to DLQ file Exception: ",e);


We should emit metrics on this exception.

@dinujoh , Thanks for the review, Resolved

dinujoh · 2023-08-24T13:58:13Z

...ink/src/main/java/org/opensearch/dataprepper/plugins/sink/prometheus/dlq/DlqPushHandler.java

+
+            dlqWriter.write(Arrays.asList(dlqObject), pluginSetting.getPipelineName(), pluginId);
+        } catch (final IOException e) {
+            LOG.error("Exception while writing failed data to DLQ, Exception : ", e);


Same as above. Emit metric on this exception.

@dinujoh , Thanks for the review, Resolved

dinujoh · 2023-08-24T14:00:27Z

...ink/src/main/java/org/opensearch/dataprepper/plugins/sink/prometheus/dlq/DlqPushHandler.java

+        if(dlqFile != null) {
+            this.dlqFile = dlqFile;
+            this.objectWriter = new ObjectMapper().writer();
+        }else{
+            this.dlqProvider = getDlqProvider(pluginFactory,bucket,stsRoleArn,awsRegion,dlqPathPrefix);
+        }


Can this be injected instead of being constructed here ?

@dinujoh , Thanks for the review, Resolved

dinujoh · 2023-08-24T14:02:35Z

...n/java/org/opensearch/dataprepper/plugins/sink/prometheus/service/PrometheusSinkService.java

+                    } else {
+                        LOG.error("No valid Event type found");
+                    }
+                    bytes = message.toByteArray();


this will cause NPE if line 171 is executed. No valid event type found.

@dinujoh Thanks for the review, Resolved

…raft

Signed-off-by: mallikagogoi7 <[email protected]>

…raft

Signed-off-by: mallikagogoi7 <[email protected]>

dlvenable · 2023-09-06T14:39:28Z

data-prepper-plugins/aws-plugin-api/build.gradle

@@ -12,7 +14,7 @@ jacocoTestCoverageVerification {
    violationRules {
        rule {
            limit {
-                minimum = 1.0
+                minimum = 0.1


This project has 100% coverage and can retain that. Dropping this to 10% will prevent us from ensuring that code coverage exists within the project. We should test the AwsRequestSigningApacheInterceptor. I believe that a different PR may be doing something similar here.

@dlvenable Resolved

dlvenable · 2023-09-06T14:41:41Z

...pensearch/dataprepper/plugins/sink/prometheus/configuration/PrometheusSinkConfiguration.java

+    @JsonProperty("encoding")
+    private String encoding = DEFAULT_ENCODING;
+
+    @JsonProperty("content-type")


This project uses underscores for YAML configuration property names. Please change to content_type.

@dlvenable Resolved

dlvenable · 2023-09-06T14:41:50Z

...pensearch/dataprepper/plugins/sink/prometheus/configuration/PrometheusSinkConfiguration.java

+    @JsonProperty("content-type")
+    private String contentType = DEFAULT_CONTENT_TYPE;
+
+    @JsonProperty("remote-write-version")


Please change to remote_write_version.

@dlvenable Resolved

dlvenable · 2023-09-06T14:42:08Z

.../org/opensearch/dataprepper/plugins/sink/prometheus/configuration/AuthenticationOptions.java

@@ -8,10 +8,10 @@

 public class AuthenticationOptions {

-    @JsonProperty("http_basic")
+    @JsonProperty("http-basic")


Please change this back to http_basic.

@dlvenable Resolved

dlvenable · 2023-09-06T14:42:15Z

.../org/opensearch/dataprepper/plugins/sink/prometheus/configuration/AuthenticationOptions.java

    private BasicAuthCredentials httpBasic;

-    @JsonProperty("bearer_token")
+    @JsonProperty("bearer-token")


Please change this back to bearer_token.

@dlvenable Resolved

Signed-off-by: mallikagogoi7 <[email protected]>

…raft

Signed-off-by: mallikagogoi7 <[email protected]>

dlvenable

This feature is incomplete, but having the code in the repo would be easier than keeping it in this PR. Somebody could pick up from the merged code.

…rge of PR opensearch-project#3103. Signed-off-by: David Venable <[email protected]>

Cleaning up some unnecessary code and dependencies from the recent merge of PR #3103. Adds missing certificate and key files to fix failures from recent merge of PR #3103. Signed-off-by: David Venable <[email protected]>

Prometheus Sink draft code for issue opensearch-project#1744.

a166841

Signed-off-by: mallikagogoi7 <[email protected]>

mallikagogoi7 requested review from chenqi0805, engechas, graytaylor0, dinujoh, kkondaka, cmanning09, asifsmohammed, dlvenable and oeyh as code owners August 2, 2023 14:26

mallikagogoi7 added 7 commits August 4, 2023 21:40

Merge branch 'main' into feature-prometheus-sink-draft

f6c6177

Signed-off-by: mallikagogoi7 <[email protected]>

Merge branch 'opensearch-project:main' into feature-prometheus-sink-d…

078228b

…raft

Merge branch 'opensearch-project:main' into feature-prometheus-sink-d…

bcca0a4

…raft

Added metrics name in Prometheus Sink for issue opensearch-project#1744.

8882a83

Signed-off-by: mallikagogoi7 <[email protected]>

Added junit test for Prometheus Sink for issue opensearch-project#1744.

51e7f44

Signed-off-by: mallikagogoi7 <[email protected]>

Added test cases and README for Prometheus Sink for issue opensearch-…

228991d

…project#1744. Signed-off-by: mallikagogoi7 <[email protected]>

Merge branch 'opensearch-project:main' into feature-prometheus-sink-d…

7b5f1c4

…raft

dinujoh requested changes Aug 14, 2023

View reviewed changes

dinujoh reviewed Aug 14, 2023

View reviewed changes

mallikagogoi7 added 2 commits August 17, 2023 13:46

Merge branch 'opensearch-project:main' into feature-prometheus-sink-d…

3d3cca6

…raft

Incorporated review comments for issue opensearch-project#1744.

f575689

Signed-off-by: mallikagogoi7 <[email protected]>

mallikagogoi7 added 4 commits August 18, 2023 15:50

Resolved review comments for issue opensearch-project#1744.

c9692a8

Signed-off-by: mallikagogoi7 <[email protected]>

Merge branch 'opensearch-project:main' into feature-prometheus-sink-d…

225f931

…raft

Fixed Failed Response code for issue opensearch-project#1744.

cbeb4b6

Signed-off-by: mallikagogoi7 <[email protected]>

Fixed Failed Response code for issue opensearch-project#1744.

e9ea0c2

Signed-off-by: mallikagogoi7 <[email protected]>

dinujoh reviewed Aug 24, 2023

View reviewed changes

mallikagogoi7 added 3 commits August 25, 2023 14:47

Merge branch 'opensearch-project:main' into feature-prometheus-sink-d…

af83962

…raft

Review Comments Resolved for issue opensearch-project#1744.

cbbc511

Signed-off-by: mallikagogoi7 <[email protected]>

AwsSigv4 Code added for opensearch-project#1744.

7693d9d

Signed-off-by: mallikagogoi7 <[email protected]>

mallikagogoi7 added 6 commits August 28, 2023 17:38

AwsSigv4 Code Bug resolved for opensearch-project#1744.

8e1d7f5

Signed-off-by: mallikagogoi7 <[email protected]>

AwsSigv4 Code Bug Fix for opensearch-project#1744.

478a495

Signed-off-by: mallikagogoi7 <[email protected]>

README file updated for opensearch-project#1744.

a671ad7

Signed-off-by: mallikagogoi7 <[email protected]>

Bug fixed for opensearch-project#1744.

e4401bd

Signed-off-by: mallikagogoi7 <[email protected]>

Merge branch 'opensearch-project:main' into feature-prometheus-sink-d…

869db0c

…raft

Fix is date for opensearch-project#1744.

1a8cb7e

Signed-off-by: mallikagogoi7 <[email protected]>

dlvenable requested changes Sep 6, 2023

View reviewed changes

mallikagogoi7 added 9 commits September 8, 2023 15:05

Merge conflict resolved for issue opensearch-project#1744.

3e79395

Signed-off-by: mallikagogoi7 <[email protected]>

Review comment resolved for opensearch-project#1744.

c59cd3d

Signed-off-by: mallikagogoi7 <[email protected]>

Added insecure changes for opensearch-project#1744.

b7f95be

Signed-off-by: mallikagogoi7 <[email protected]>

Merge branch 'opensearch-project:main' into feature-prometheus-sink-d…

4947506

…raft

Merge branch 'opensearch-project:main' into feature-prometheus-sink-d…

b0f2737

…raft

Merge branch 'opensearch-project:main' into feature-prometheus-sink-d…

b3b9cec

…raft

Merge branch 'opensearch-project:main' into feature-prometheus-sink-d…

1db7b44

…raft

Merge branch 'opensearch-project:main' into feature-prometheus-sink-d…

d9248f8

…raft

Insecure Skip Verify resolved for opensearch-project#1744.

9710415

Signed-off-by: mallikagogoi7 <[email protected]>

dlvenable approved these changes Dec 11, 2023

View reviewed changes

dinujoh approved these changes Dec 11, 2023

View reviewed changes

dlvenable merged commit 073419f into opensearch-project:main Dec 11, 2023
22 of 24 checks passed

dlvenable added a commit to dlvenable/data-prepper that referenced this pull request Dec 11, 2023

Cleaning up some unnecessary code and dependencies from the recent me…

71f63e1

…rge of PR opensearch-project#3103. Signed-off-by: David Venable <[email protected]>

dlvenable mentioned this pull request Dec 11, 2023

Clean-up from recent merge of PR #3103 #3843

Merged

4 tasks

dlvenable added a commit to dlvenable/data-prepper that referenced this pull request Dec 11, 2023

Adds missing certificate and key files to fix failures from recent me…

7419893

…rge of PR opensearch-project#3103. Signed-off-by: David Venable <[email protected]>


		- `url` The http/https endpoint url which can bee backed by prometheus.

		- `encoding` Default is snappy


		- `scope` (Optional) : This scope limit an application's access to a user's account.

		- `aws_sigv4`: A boolean flag to sign the HTTP request with AWS credentials. Default to `false`.

		Certificate trustedCa;
		trustedCa = factory.generateCertificate(certificate);

Prometheus Sink draft code for issue #1744. #3103

Prometheus Sink draft code for issue #1744. #3103

Conversation

mallikagogoi7 commented Aug 2, 2023

Description

Issues Resolved

Check List

dinujoh left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mallikagogoi7 commented Aug 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlvenable left a comment

Choose a reason for hiding this comment

dinujoh left a comment •

edited

Loading