[receiver/awsfirehosereceiver] Add support for CloudWatch logs #35077

gavincabbage · 2024-09-09T16:24:29Z

Description: Add support for CloudWatch logs in JSON format.

The receiver can now be configured to receiver either cwmetrics or cwlogs record type for CloudWatch metrics logs, respectively. The former is only valid for metrics receivers and the latter for logs receivers.

Some changes were necessary to the default record type handling to conform to the contracts required by the generated tests and ensure the default config returned by the factory was usable. Previously the default record type was cwmetrics but this is not sensible for logs receivers.

To accommodate this, the default record type is now telemetry-type-specific. That is, for metrics it is cwmetrics and for logs it is cwlogs. Each receiver makes this determination on its own. Therefore an empty string is now the default returned by the factory and an empty string is also acceptable in the users config. It is no longer required to specify a record type, although it is still supported, so this is not a breaking change.

Link to tracking Issue: n/a

Testing: Unit tests covering new code as well as manual smoke testing against AWS Firehose and CloudWatch themselves.

Documentation: Component README update.

linux-foundation-easycla · 2024-09-09T16:24:34Z

The committers listed above are authorized under a signed CLA.

✅ login: gavincabbage / name: Gavin Cabbage (61cb91b, b4b04dc, dc50db2, 5c19051, de1dca3, 12a1829, f94f35b)

mwear

I made a first pass through this PR. It very closely mirrors what currently exists for metrics. I would like to have eyes from someone with more expertise on this. @Aneurysm9 can you possibly take a look or recommend someone who can?

receiver/awsfirehosereceiver/internal/unmarshaler/cwlog/cwlog.go

receiver/awsfirehosereceiver/logs_receiver.go

…eedback

gavincabbage · 2024-10-01T17:36:02Z

Thanks for the feedback @mwear! I believe addressed your comments and the test failures.

I did a bit of refactoring around the default record type logic to conform to the contracts required by the generated tests and have updated the description accordingly.

gavincabbage · 2024-10-02T14:05:18Z

There are a couple CI failures still but it is unclear to me what the problem is.

jackgopack4 · 2024-10-02T16:42:21Z

There are a couple CI failures still but it is unclear to me what the problem is.

so for build-and-test/checks the test output says "run make generate and commit the changes"

I tried that on my local machine and it generated some changes to receiver/awsfirehosereceiver/generated_component_test.go and receiver/awsfirehosereceiver/generated_package_test.go

Might not solve the cross-compile stage but maybe would check off one of the failing tests

jackgopack4

left suggestions for what the make generate command outputted on my machine. Looks like that might help you pass the build-and-test/check stage

jackgopack4 · 2024-09-30T18:25:16Z

receiver/awsfirehosereceiver/internal/unmarshaler/cwlog/unmarshaler.go

+		}
+		// Multiple logs in each record separated by newline character
+		for datumIndex, datum := range bytes.Split(record, []byte(recordDelimiter)) {
+			if len(datum) > 0 {


will datum ever be an invalid JSON? If so, could be worth checking to ensure it is a valid JSON before calling json.Unmarshal on line 58

I think it is unlikely as this is the wrapper coming from CloudWatch and not the user's log payload itself (which could be anything).

Out of curiosity though, how would you check for valid JSON besides trying to unmarshal it?

You're right as long as unmarshal has enough safeguards it's probably fine.

jackgopack4 · 2024-09-30T18:26:21Z

receiver/awsfirehosereceiver/internal/unmarshaler/cwlog/unmarshaler.go

+}
+
+// isValid validates that the cWMetric has been unmarshalled correctly.
+func (u Unmarshaler) isValid(log cwLog) bool {


are there any other requirements for correctness here, or is nonempty fields sufficient?

I don't think so. All these attributes are AWS IDs so they're arbitrary strings. I based this validation on what I saw the metric path doing. That path adds more attributes than I have here because metric payloads have more information (e.g. region, which is not included in logs payloads) but otherwise followed the established pattern.

jackgopack4 · 2024-09-30T18:28:08Z

receiver/awsfirehosereceiver/logs_receiver.go

+	"github.com/open-telemetry/opentelemetry-collector-contrib/receiver/awsfirehosereceiver/internal/unmarshaler"
+)
+
+// The logsConsumer implements the firehoseConsumer


Suggested change

// The logsConsumer implements the firehoseConsumer

// A logsConsumer implements the firehoseConsumer

Omitted The/A entirely to follow the usual godoc approach of starting with the subject name (not that it matters much for an unexported type).

jackgopack4 · 2024-09-30T18:31:38Z

receiver/awsfirehosereceiver/testdata/cwlogs_config.yaml

@@ -0,0 +1,7 @@
+awsfirehose:
+  endpoint: 0.0.0.0:4433


since the default endpoint is localhost now instead of 0.0.0.0, would it work with localhost?

#34394

Sorry I missed this question.

This receiver works just fine with localhost with the caveat that Firehose needs to be able to reach it (Firehose also requires an HTTPS endpoint, I believe). In my testing I've used ngrok to facilitate this.

receiver/awsfirehosereceiver/internal/unmarshaler/cwlog/compression/compression.go

receiver/awsfirehosereceiver/generated_component_test.go

receiver/awsfirehosereceiver/generated_package_test.go

gavincabbage · 2024-10-02T18:57:41Z

Thanks @jackgopack4! Sorry I missed that bit about make generate 🤦

Besides the cross compile issue, I think I've addressed everything you called out, with either a change or a question/response.

jackgopack4

lgtm, thanks for humoring my suggestions

…telemetry#35077) **Description:** Add support for CloudWatch logs in JSON format. The receiver can now be configured to receiver either `cwmetrics` or `cwlogs` record type for CloudWatch metrics logs, respectively. The former is only valid for metrics receivers and the latter for logs receivers. Some changes were necessary to the default record type handling to conform to the contracts required by the generated tests and ensure the default config returned by the factory was usable. Previously the default record type was `cwmetrics` but this is not sensible for logs receivers. To accommodate this, the default record type is now telemetry-type-specific. That is, for metrics it is `cwmetrics` and for logs it is `cwlogs`. Each receiver makes this determination on its own. Therefore an empty string is now the default returned by the factory and an empty string is also acceptable in the users config. It is no longer required to specify a record type, although it is still supported, so this is not a breaking change. **Link to tracking Issue:** n/a **Testing:** Unit tests covering new code as well as manual smoke testing against AWS Firehose and CloudWatch themselves. **Documentation:** Component README update.

gavincabbage changed the title ~~receiver/awsfirehosereceiver: add cwlogs~~ [receiver/awsfirehosereceiver] Add support for CloudWatch logs Sep 9, 2024

github-actions bot added the receiver/awsfirehose label Sep 9, 2024

github-actions bot requested a review from Aneurysm9 September 9, 2024 16:35

gavincabbage force-pushed the awsfirehosereceiver/cwlogs branch 2 times, most recently from d3d4f74 to 977ffa7 Compare September 9, 2024 16:52

gavincabbage force-pushed the awsfirehosereceiver/cwlogs branch from 977ffa7 to 9739a67 Compare September 17, 2024 13:59

gavincabbage marked this pull request as ready for review September 17, 2024 13:59

gavincabbage requested review from a team and dashpole September 17, 2024 13:59

github-actions bot assigned andrzej-stencel Sep 17, 2024

gavincabbage force-pushed the awsfirehosereceiver/cwlogs branch from 9739a67 to 60e448f Compare September 19, 2024 13:18

gavincabbage requested a review from a team as a code owner September 19, 2024 13:18

[receiver/awsfirehosereceiver] Add support for CloudWatch logs

5c19051

gavincabbage force-pushed the awsfirehosereceiver/cwlogs branch from 60e448f to 5c19051 Compare September 26, 2024 20:19

mwear reviewed Sep 30, 2024

View reviewed changes

gavincabbage added 2 commits October 1, 2024 13:21

[awsfirehosereceiver/cwlogs] Refactor default record type logic; PR f…

b4b04dc

…eedback

[awsfirehosereceiver/cwlogs] Correct comments

dc50db2

gavincabbage added 2 commits October 1, 2024 14:53

[awsfirehosereceiver/cwlogs] Remove unused constants

12a1829

[awsfirehosereceiver/cwlogs] Remove unconventional common attributes

de1dca3

jackgopack4 suggested changes Oct 2, 2024

View reviewed changes

gavincabbage added 2 commits October 2, 2024 14:29

[awsfirehosereceiver/cwlogs] Run make generate

61cb91b

[awsfirehosereceiver/cwlogs] Doc fixes; add additional common attributes

f94f35b

jackgopack4 approved these changes Oct 2, 2024

View reviewed changes

mwear approved these changes Oct 9, 2024

View reviewed changes

dmitryax merged commit 9dbe5e0 into open-telemetry:main Oct 9, 2024
156 checks passed

github-actions bot added this to the next release milestone Oct 9, 2024

gavincabbage mentioned this pull request Oct 24, 2024

[receiver/awsfirehosereceiver] make otlp_v1 a valid record type #35750

Merged

Aneurysm9 mentioned this pull request Jan 15, 2025

[receiver/awsfirehosereceiver] Support receiving multiple record type using single endpoint #35988

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[receiver/awsfirehosereceiver] Add support for CloudWatch logs #35077

[receiver/awsfirehosereceiver] Add support for CloudWatch logs #35077

gavincabbage commented Sep 9, 2024 •

edited

Loading

linux-foundation-easycla bot commented Sep 9, 2024 •

edited

Loading

mwear left a comment

gavincabbage commented Oct 1, 2024

gavincabbage commented Oct 2, 2024

jackgopack4 commented Oct 2, 2024

jackgopack4 left a comment

jackgopack4 Sep 30, 2024

gavincabbage Oct 2, 2024

jackgopack4 Oct 2, 2024

jackgopack4 Sep 30, 2024

gavincabbage Oct 2, 2024

jackgopack4 Sep 30, 2024

gavincabbage Oct 2, 2024

jackgopack4 Sep 30, 2024

gavincabbage Oct 4, 2024

gavincabbage commented Oct 2, 2024

jackgopack4 left a comment

	// The logsConsumer implements the firehoseConsumer
	// A logsConsumer implements the firehoseConsumer

[receiver/awsfirehosereceiver] Add support for CloudWatch logs #35077

[receiver/awsfirehosereceiver] Add support for CloudWatch logs #35077

Conversation

gavincabbage commented Sep 9, 2024 • edited Loading

linux-foundation-easycla bot commented Sep 9, 2024 • edited Loading

mwear left a comment

Choose a reason for hiding this comment

gavincabbage commented Oct 1, 2024

gavincabbage commented Oct 2, 2024

jackgopack4 commented Oct 2, 2024

jackgopack4 left a 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

gavincabbage commented Oct 2, 2024

jackgopack4 left a comment

Choose a reason for hiding this comment

gavincabbage commented Sep 9, 2024 •

edited

Loading

linux-foundation-easycla bot commented Sep 9, 2024 •

edited

Loading