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

fix(awscloudwatchreceiver): emit logs from one stream in one resource #22976

Merged
merged 5 commits into from
Jul 20, 2023

Conversation

aboguszewski-sumo
Copy link
Member

Description:
The receiver treated every event as a separate log. However, it makes a lot sense to group logs into common resource by their group and stream. Logs without a stream are grouped into the same resource.
I'm not sure how should scope be treated here, but my intuition tells me that it makes sense to group them also into the same scope.

Link to tracking Issue:
Fixes #22145

Testing:
The unit test for the receiver has beed updated. Note that the events in prefixed.yaml are the same, but differ by their id - I found it simpler to do than creating different events.

@aboguszewski-sumo
Copy link
Member Author

@djaglowski @schmikei

@aboguszewski-sumo
Copy link
Member Author

@djaglowski

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, just a couple minor notes

receiver/awscloudwatchreceiver/logs.go Outdated Show resolved Hide resolved
receiver/awscloudwatchreceiver/logs.go Outdated Show resolved Hide resolved
@aboguszewski-sumo
Copy link
Member Author

Sorry that it took so long, but I fixed these issues now.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jul 20, 2023
@aboguszewski-sumo
Copy link
Member Author

@djaglowski this got marked as stale and is ready to review

@djaglowski djaglowski removed the Stale label Jul 20, 2023
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Apologies for missing this. This looks functionally correct to me but I have one suggestion about how to make the code clearer.

receiver/awscloudwatchreceiver/logs.go Outdated Show resolved Hide resolved
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Just one nit - I think we can avoid an extra allocation

receiver/awscloudwatchreceiver/logs.go Outdated Show resolved Hide resolved
Co-authored-by: Daniel Jaglowski <[email protected]>
@djaglowski djaglowski merged commit 7ba5b5c into open-telemetry:main Jul 20, 2023
89 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The AWS cloudwatch receiver treats every cloudwatch log as a new resource log
3 participants