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

Wire up awsxrayproxy extension #5747

Merged
merged 7 commits into from
Oct 19, 2021
Merged

Conversation

anuraaga
Copy link
Contributor

Description:

Adds awsxrayproxy to the contrib components

Link to tracking Issue:

Testing:

Documentation:

@anuraaga anuraaga requested review from a team and bogdandrutu October 14, 2021 05:25
@jpkrohling
Copy link
Member

Looks like this isn't the only component that isn't wired yet:

$ ls -1 exporter/ > /tmp/exporters-available.txt
$ grep "github.com/open-telemetry/opentelemetry-collector-contrib" internal/components/components.go | sed 's/"//g' | grep exporter | awk -F/ '{print $5}' > /tmp/exporters-wired.txt
$ diff /tmp/exporters-*
2d1
< awscloudwatchlogsexporter
12d10
< elasticsearchexporter
16d13
< googlecloudpubsubexporter
26d22
< observiqexporter
33d28
< skywalkingexporter

Would it make sense to wire them all in a single PR?

@jpkrohling
Copy link
Member

Extensions and processors seem to be in sync, but receivers aren't:

$ diff /tmp/receivers-*
1d0
< awscontainerinsightreceiver
5d3
< cloudfoundryreceiver
11,12d8
< googlecloudpubsubreceiver
< googlecloudspannerreceiver
14d9
< httpdreceiver
23,26d17
< memcachedreceiver
< mongodbatlasreceiver
< mysqlreceiver
< nginxreceiver
34d24
< scraperhelper

@anuraaga
Copy link
Contributor Author

I would need to dig through whether any of those are in-progress (as my awsxrayproxy was until this PR :) ). The guidelines recommend only wiring in when ready

https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#when-adding-a-new-component

@jpkrohling
Copy link
Member

Sounds good, I'll open a separate issue to track that. I thought this was in connection to #5725 :-)

@jcchavezs
Copy link
Contributor

Looks good @anuraaga

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

Do we want to rename to awsproxy in case we want any future service requests proxied through here? Won't block on it since i can't think of any use cases right now, but just a thought for extensibility.

@bogdandrutu
Copy link
Member

Please rebase.

@anuraaga anuraaga requested a review from Aneurysm9 as a code owner October 18, 2021 04:33
@anuraaga
Copy link
Contributor Author

Thanks went ahead and renamed to awsproxy as suggested

@bogdandrutu
Copy link
Member

Please rebase

@bogdandrutu
Copy link
Member

@anuraaga not sure if it was a race, but another merge is required :(

@bogdandrutu bogdandrutu merged commit 0a8bed1 into open-telemetry:main Oct 19, 2021
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.

5 participants