Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AWS CloudWatch Kamelet #123

Merged
merged 1 commit into from
Jun 15, 2021
Merged

Conversation

claudio4j
Copy link
Contributor

#116
When regenerating the doc, it updated the aws s3 doc.

@oscerd
Copy link
Contributor

oscerd commented Apr 8, 2021

Lgtm cc @nicolaferraro

Copy link
Member

@nicolaferraro nicolaferraro left a comment

Choose a reason for hiding this comment

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

Some issues highlighted.. open to different suggestions...

Comment on lines 20 to 26
- CamelAwsCwMetricNamespace
- CamelAwsCwMetricName
- CamelAwsCwMetricValue
- CamelAwsCwMetricUnit
- CamelAwsCwMetricTimestamp
- CamelAwsCwMetricDimensionName
- CamelAwsCwMetricDimensionValue
Copy link
Member

Choose a reason for hiding this comment

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

As general rule, we don't use Camel* headers in Kamelets, since these are higher level objects that hide their Camel engine. Usually we have a few headers, but here we have many more of them, so the mapping may be verbose, since e.g. we may need to detect both e.g. metric-namespace and ce-metric-namespace headers.

One reason is that we want to hide camel, the other is that in some contexts, e.g. knative, you are able to only send cloud-event extensions, which are mapped to headers that start with the "ce-" prefix.

A different issue is with the metric-timestamp header. Consider that (from the doc) CamelAwsCwMetricTimestamp needs to be a java.util.Date. When this sink is bound to a Knative channel or a Kafka topic, all headers are textual, so we need to define the format of the header and its conversion from text to date.

Out of curiosity, is the body of the incoming message used? Maybe we should we use a structured body instead of relying on headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As general rule, we don't use Camel* headers in Kamelets,

Kamelets should not expose those camel headers,but can set them in - set-property steps ?

The incoming body is empty, the camel-aws2-cw component uses only the values from the header or query parameters.
Would the structured body be something like this ?

{namespace: "my-aws-ns",
metric-name: "my-metric",
metric-value: "1.2",
...
}

Then the to:() directive would use the body structure like the following ?

.to("aws2-cw:${body[namespace]}?name=${body[metric-name]}")

About the timestamp conversion, would it be using the simple("${headerAs(key, type)}") ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we may use more user-friendly names for the properties by remapping them in set-property steps, as other kamelets are doing.

There are many possibilities about the body, we can keep it empty. Or we can expect the body to contain the metric-value header if that is missing. Or, we can detect if the body is a simple value or a structured JSON, in which case we can unmarshal it and use set-header with simple or jsonpath to get camel headers from the structured body.
Anyway, it's also ok to rely on headers for a first release, then add a different logic in the future. Sounds like it's better to improve the component to optionally accept a structured body rather than adding it here.

The timestamp conversion may be possible with simple. But looking here https://camel.apache.org/components/latest/languages/simple-language.html, I see only methods for formatting. I think e.g. "number of milliseconds since epoch" as java.lang.Long can be automatically converted by Camel to java.util.Date, so we may use that format here for simplicity.

description: The AWS region to connect to.
type: string
example: eu-west-1
metricName:
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these options on the endpoint can be set as headers first. Headers have precedence over the endpoint option. https://github.com/apache/camel/blob/master/components/camel-aws/camel-aws2-cw/src/main/java/org/apache/camel/component/aws2/cw/Cw2Producer.java#L124

So it would be better to set all of this as property or header, like here:
https://github.com/apache/camel-kamelets/blob/master/azure-eventhubs-sink.kamelet.yaml#L57

Especially the metric value would never be constant, so it doesn't make really sense to set it as an endpoint option.

I suggest to set all the dynamic fields as header or property following the example I reported above.

@oscerd
Copy link
Contributor

oscerd commented May 27, 2021

What is the status of this @claudio4j ?

@claudio4j
Copy link
Contributor Author

What is the status of this @claudio4j ?

I had other priorities and put this work on hold. But I am back into it today and expect to push the changes soon.

@claudio4j
Copy link
Contributor Author

@oscerd I pushed the required changes, can you review ?

@claudio4j claudio4j force-pushed the add_aws_cloudwatch branch 2 times, most recently from d2f54fe to ef5dac5 Compare June 1, 2021 18:34
Copy link
Contributor

@oscerd oscerd left a comment

Choose a reason for hiding this comment

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

@nicolaferraro please have a look again. Looks good to me.

@claudio4j
Copy link
Contributor Author

@nicolaferraro can you review this kamelet ?

@nicolaferraro
Copy link
Member

Thanks, can you check the conflicts?

@claudio4j
Copy link
Contributor Author

Thanks, can you check the conflicts?

Done. Thanks.

@oscerd oscerd merged commit f9a150b into apache:main Jun 15, 2021
@oscerd
Copy link
Contributor

oscerd commented Jun 15, 2021

Thanks.

@claudio4j claudio4j deleted the add_aws_cloudwatch branch June 15, 2021 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants