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.

Sign up for GitHub

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

Jump to bottom

[processor/awsapplicationsignalsprocessor] Create No-Op processor skeleton for AWS Application Signals Processor #33432

Closed
wants to merge 12 commits into from
Closed

[processor/awsapplicationsignalsprocessor] Create No-Op processor skeleton for AWS Application Signals Processor #33432

wants to merge 12 commits into from

Conversation

jj22ee
Copy link
Contributor

@jj22ee jj22ee commented Jun 7, 2024 •
edited
Loading

Description:

The PR adds an initial skeleton implementation of the awsapplicationsignalsprocessor, which is currently a No-Op implementation for the processor for traces+metrics.

  • The intention of the AWS Application Signals processor is used to reduce the cardinality of telemetry metrics and traces before exporting them to CloudWatch Logs via EMF and X-Ray respectively.

A series of PRs will follow for the full implementation of this new processor.

Link to tracking Issue:
#32808

Testing:

Documentation:

@jj22ee jj22ee requested review from a team and MovieStoreGuy June 7, 2024 16:21
@jj22ee jj22ee force-pushed the appsignals-processor-part-1-skeleton branch from 27a80f8 to cafe6a9 Compare June 7, 2024 20:30
@mx-psi mx-psi assigned bryan-aguilar and unassigned mx-psi Jun 7, 2024
tiffany76
tiffany76 reviewed Jun 11, 2024
View reviewed changes
Copy link
Contributor

@tiffany76 tiffany76 left a comment

Choose a reason for hiding this comment

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

Docs reviewer here. Just a few small style suggestions. Thanks!

processor/awsapplicationsignalsprocessor/README.md Outdated Show resolved Hide resolved
processor/awsapplicationsignalsprocessor/README.md Outdated Show resolved Hide resolved
@jj22ee @tiffany76
Apply README suggestions from code review
f572a10 
Co-authored-by: Tiffany Hrabusa <30397949+tiffany76@users.noreply.github.com>
mxiamxia
mxiamxia reviewed Jun 11, 2024
View reviewed changes
.github/CODEOWNERS Outdated Show resolved Hide resolved
@jj22ee jj22ee mentioned this pull request Jun 12, 2024
Closed
@github-actions 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 Jun 27, 2024
@jj22ee
Copy link
Contributor Author

jj22ee commented Jun 27, 2024 •
edited
Loading

Hi @MovieStoreGuy, can you help please approve/merge this PR?
We don't have a new sponsor as our previous sponsor is gone, but we have approvals from AWS Observability team.

@github-actions github-actions bot removed the Stale label Jun 28, 2024
MovieStoreGuy
MovieStoreGuy reviewed Jul 1, 2024
View reviewed changes
Copy link
Contributor

@MovieStoreGuy MovieStoreGuy left a comment

Choose a reason for hiding this comment

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

Just some few minor nits as glance through, you will need to find a sponsor as part of the issue raised for this PR.

processor/awsapplicationsignalsprocessor/factory.go Outdated Show resolved Hide resolved
processor/awsapplicationsignalsprocessor/factory.go Outdated Show resolved Hide resolved
processor/awsapplicationsignalsprocessor/processor.go

type awsapplicationsignalsprocessor struct{}

func (ap *awsapplicationsignalsprocessor) StartMetrics(_ context.Context, _ component.Host) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any particular reason why StartMetrics and StartTraces are not combined as Start ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a new processor is created via a factory (via NewFactory()), providing processor logic for traces and metrics is done separately (via processor.WithTraces and processor.WithMetrics)

So, StartMetrics and StartTraces are for the respective processorhelper.NewMetricsProcessor and processorhelper.NewTracesProcessor, for the respective metrics and traces processor.

@github-actions 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 23, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 7, 2024

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MovieStoreGuy MovieStoreGuy MovieStoreGuy left review comments

@tiffany76 tiffany76 tiffany76 left review comments

@wangzlei wangzlei wangzlei approved these changes

@mxiamxia mxiamxia mxiamxia approved these changes

Assignees

@bryan-aguilar bryan-aguilar

Labels
Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jj22ee @mxiamxia @tiffany76 @MovieStoreGuy @wangzlei @mx-psi @bryan-aguilar