-
Notifications
You must be signed in to change notification settings - Fork 486
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 otel.receiver.aws_firehose
component
#6005
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening your first PR on the grafana/agent repo @Obito1903 🙌
I've left some comments, mainly documentation nits as well as a comment about not needing to wire up the static mode; we're on a good path here!
component/all/all.go
Outdated
@@ -83,6 +83,7 @@ import ( | |||
_ "github.com/grafana/agent/component/otelcol/processor/span" // Import otelcol.processor.span | |||
_ "github.com/grafana/agent/component/otelcol/processor/tail_sampling" // Import otelcol.processor.tail_sampling | |||
_ "github.com/grafana/agent/component/otelcol/processor/transform" // Import otelcol.processor.transform | |||
_ "github.com/grafana/agent/component/otelcol/receiver/awsfirehose" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you add a comment next to this import like other components do?
pkg/traces/config.go
Outdated
@@ -901,6 +902,7 @@ func tracingFactories() (otelcol.Factories, error) { | |||
} | |||
|
|||
receivers, err := receiver.MakeFactoryMap( | |||
awsfirehosereceiver.NewFactory(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need this line; it would only make sense for static mode where we haven't wired up the rest of the code or documented as an option. cc @ptodev
|
||
Name | Type | Description | Default | Required | ||
---- | ---- | ----------- | ------- | -------- | ||
`record_type` | `string` | The type of record being received from the delivery stream. | `cwmetrics` | no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also document AccessKey here?
`otelcol.receiver.awsfirehose` accepts cloudwatch json-formatted traces over the network and | ||
forwards it to other `otelcol.*` components. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The upstream component mentions this as its intro:
Receiver for ingesting AWS Kinesis Data Firehose delivery stream messages and parsing the records received based on the configured record type.
Are the two statements compatible (I'm not familiar with firehose tbh). Also, just to make sure, does this receiver receive JSON-formatted traces and forwards metrics generated out of them?
Finally, the upstream docs mention that:
The AWS Kinesis Data Firehose Delivery Streams currently only support HTTPS endpoints using port 443. This can be potentially circumvented using a Load Balancer.
Is it applicable to this component as well? Should we copy this over?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To answer your first question, yes, the two statements are compatible, as AWS Kinesis Data Firehose Delivery Streams is the service used by Cloudwatch to send metrics streams to an HTTP endpoint. I will clarify it in the doc.
For the second point, I did not explain it clearly. The receiver receives JSON-formatted metrics from AWS and forward them to other otel components.
As for the HTTPS-only endpoint, since it's a requirement from the AWS side, I will add the information to the doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some documentation suggestions...
docs/sources/flow/reference/components/otelcol.receiver.awsfirehose.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/otelcol.receiver.awsfirehose.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/otelcol.receiver.awsfirehose.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/otelcol.receiver.awsfirehose.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/otelcol.receiver.awsfirehose.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/otelcol.receiver.awsfirehose.md
Outdated
Show resolved
Hide resolved
@tpaschalis Is this one ready to move to the next stage of approvals? |
@clayton-cornell yes, LGTM from a code perspective |
Will have to look into the checksum mismatch being reported here, it's weird |
d4b38ef
to
f98b484
Compare
docs/sources/flow/reference/components/otelcol.receiver.awsfirehose.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/otelcol.receiver.awsfirehose.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/otelcol.receiver.awsfirehose.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/otelcol.receiver.awsfirehose.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/otelcol.receiver.awsfirehose.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Paschalis Tsilias <[email protected]>
Co-authored-by: Paschalis Tsilias <[email protected]>
…ehose.md Co-authored-by: Clayton Cornell <[email protected]>
…ehose.md Co-authored-by: Clayton Cornell <[email protected]>
…ehose.md Co-authored-by: Clayton Cornell <[email protected]>
…ehose.md Co-authored-by: Clayton Cornell <[email protected]>
…ehose.md Co-authored-by: Clayton Cornell <[email protected]>
…ehose.md Co-authored-by: Clayton Cornell <[email protected]>
…ehose.md Co-authored-by: Paulin Todev <[email protected]>
…ehose.md Co-authored-by: Paulin Todev <[email protected]>
…ehose.md Co-authored-by: Paulin Todev <[email protected]>
…ehose.md Co-authored-by: Paulin Todev <[email protected]>
Signed-off-by: Paschalis Tsilias <[email protected]>
a9ab0f4
to
4279489
Compare
Signed-off-by: Paschalis Tsilias <[email protected]>
Signed-off-by: Paschalis Tsilias <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I've taken the liberty to fix various chores around documentation to make this easier to merge.
I'd like to let @ptodev take a final look and merge this.
otel.receiver.awsfirehose
componentotel.receiver.aws_firehose
component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noted a few issues which need fixing. Other than that, it looks ok to me.
docs/sources/flow/reference/components/otelcol.receiver.aws_firehose.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this PR change the loki.source
component docs? It's probably by accident?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I accidentally did this as I was trying to make the check for compatible components pass, I'll revert.
docs/sources/flow/reference/components/otelcol.receiver.aws_firehose.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/otelcol.receiver.aws_firehose.md
Outdated
Show resolved
Hide resolved
Signed-off-by: Paschalis Tsilias <[email protected]>
Signed-off-by: Paschalis Tsilias <[email protected]>
Signed-off-by: Paschalis Tsilias <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR will also need a change similar to the one here:
https://github.com/grafana/agent/pull/6475/files
…rehose.md Co-authored-by: Paulin Todev <[email protected]>
Signed-off-by: Paschalis Tsilias <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note on aliases. I don't think we really need to add them on a new topic.. only if the topic is renamed/moved after publishing.
All the Cloud aliases are for historical mount points for topics that were published when things got shuffled in Cloud. Since this is a new topic... we can drop them until things get shuffled again. :-)
aliases: | ||
- /docs/grafana-cloud/agent/flow/reference/components/otelcol.receiver.aws_firehose/ | ||
- /docs/grafana-cloud/monitor-infrastructure/agent/flow/reference/components/otelcol.receiver.aws_firehose/ | ||
- /docs/grafana-cloud/monitor-infrastructure/integrations/agent/flow/reference/components/otelcol.receiver.aws_firehose/ | ||
- /docs/grafana-cloud/send-data/agent/flow/reference/components/otelcol.receiver.aws_firehose/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New topic? We don't really need to add all these aliases. We only need them if the topic moves or is renamed after it has been published at least once. It's not hurting anything to add them, but they aren't necessary.
@ptodev This one seems to have been missed... is it still valid/relevant? |
PR Description
Add a new otel.receiver.awsfirehose based on the AWS Kinesis Data Firehose Receiver available in the opentelemetry collector.
Which issue(s) this PR fixes
Fixes grafana/alloy#287
Notes to the Reviewer
PR Checklist