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

Cloud Foundry receiver step 2 - implementation #5543

Merged
merged 19 commits into from
Nov 19, 2021
Merged

Cloud Foundry receiver step 2 - implementation #5543

merged 19 commits into from
Nov 19, 2021

Conversation

agoallikmaa
Copy link
Contributor

Description: Adds implementation to the Cloud Foundry receiver skeleton merged in #4626 .

Link to tracking Issue: #5320

Testing: Some implementation parts have been unit tested, the rest will be added as integration tests, in a separate PR which tests against a local HTTP server which will respond with prerecorded responses (recorded from running it against Tanzu Application Service).

Documentation: Configuration was updated in documentation to reflect using HTTPClientSettings instead of custom fields.

@agoallikmaa agoallikmaa requested review from a team and dmitryax October 1, 2021 12:30
@tigrannajaryan
Copy link
Member 

receiver/cloudfoundryreceiver/config_test.go: Import groups are not in the proper order: ["Std" "Third party" "Third party"]
receiver/cloudfoundryreceiver/factory.go: Import groups are not in the proper order: ["Std" "Third party" "Third party"]
receiver/cloudfoundryreceiver/uaa.go: Import groups are not in the proper order: ["Std" "Third party" "Third party"]

impi verification failed: Found 3 errors
make: *** [Makefile.Common:107: impi] Error 1
make: *** Waiting for unfinished jobs....
Error: Process completed with exit code 2.

@pellared pellared mentioned this pull request Oct 11, 2021
Closed
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Oct 12, 2021
@mx-psi mx-psi removed the Stale label Oct 12, 2021
@mx-psi
Copy link
Member

mx-psi commented Oct 12, 2021

@pellared can you review?

@agoallikmaa please rebase

pellared
pellared reviewed Oct 12, 2021
View reviewed changes
receiver/cloudfoundryreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/cloudfoundryreceiver/receiver.go Show resolved Hide resolved
receiver/cloudfoundryreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/cloudfoundryreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/cloudfoundryreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/cloudfoundryreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/cloudfoundryreceiver/receiver.go Outdated Show resolved Hide resolved
pellared
pellared reviewed Oct 12, 2021
View reviewed changes
receiver/cloudfoundryreceiver/uaa.go Show resolved Hide resolved
receiver/cloudfoundryreceiver/uaa.go Outdated Show resolved Hide resolved
receiver/cloudfoundryreceiver/stream.go Outdated Show resolved Hide resolved
@pellared
Copy link
Member

pellared commented Oct 12, 2021 •
edited
Loading

@dmitryax Can you please check if my comments make sense (as this is my first "real" review in this repository)? Feel free to resolve the irrelevant ones or add comments if you think that some of them are minor ("nice to address").

tigrannajaryan
tigrannajaryan reviewed Oct 13, 2021
View reviewed changes
receiver/cloudfoundryreceiver/README.md Show resolved Hide resolved
receiver/cloudfoundryreceiver/receiver.go Show resolved Hide resolved
receiver/cloudfoundryreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/cloudfoundryreceiver/receiver.go Show resolved Hide resolved
receiver/cloudfoundryreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/cloudfoundryreceiver/receiver_test.go Outdated Show resolved Hide resolved
receiver/cloudfoundryreceiver/stream.go Outdated Show resolved Hide resolved
pellared
pellared reviewed Oct 13, 2021
View reviewed changes
receiver/cloudfoundryreceiver/config.go Outdated Show resolved Hide resolved
receiver/cloudfoundryreceiver/config.go Outdated Show resolved Hide resolved
receiver/cloudfoundryreceiver/config.go Show resolved Hide resolved
receiver/cloudfoundryreceiver/converter_test.go Show resolved Hide resolved
receiver/cloudfoundryreceiver/converter_test.go Outdated Show resolved Hide resolved
pellared
pellared reviewed Oct 14, 2021
View reviewed changes
receiver/cloudfoundryreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/cloudfoundryreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/cloudfoundryreceiver/stream.go Outdated Show resolved Hide resolved
receiver/cloudfoundryreceiver/stream.go Outdated Show resolved Hide resolved
receiver/cloudfoundryreceiver/receiver.go Outdated Show resolved Hide resolved
@pellared
Copy link
Member

Can you also please add some screenshots/logs from E2E testing after you are done with all your changes?

Moreover, I think we should also great to add some documentation on how to do the E2E testing e.g. with Tanzu Application Service. It can be as a separate PR.

@pellared pellared mentioned this pull request Oct 20, 2021
Closed
pellared
pellared approved these changes Oct 21, 2021
View reviewed changes
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

A left a few last minor comments. But it is great as it is. Nice work 👍

receiver/cloudfoundryreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/cloudfoundryreceiver/uaa.go Show resolved Hide resolved
receiver/cloudfoundryreceiver/uaa.go Show resolved Hide resolved
receiver/cloudfoundryreceiver/stream_test.go Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

Mostly looks good, just a couple small comments remaining.

@agoallikmaa agoallikmaa force-pushed the cloudfoundry-receiver-impl branch 2 times, most recently from f5542f0 to 888a200 Compare November 9, 2021 08:12
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Nov 17, 2021
@pellared
Copy link
Member

@tigrannajaryan Can you review it again, please?

@mx-psi mx-psi removed the Stale label Nov 17, 2021
@mx-psi
Copy link
Member

mx-psi commented Nov 19, 2021

@agoallikmaa needs a rebase

@tigrannajaryan friendly ping to give a final review

tigrannajaryan
tigrannajaryan approved these changes Nov 19, 2021
View reviewed changes
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Can be merged once rebased.

@tigrannajaryan tigrannajaryan merged commit 068466a into open-telemetry:main Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pellared pellared pellared approved these changes

@mx-psi mx-psi mx-psi left review comments

@tigrannajaryan tigrannajaryan tigrannajaryan approved these changes

@dmitryax dmitryax Awaiting requested review from dmitryax

Assignees

@mx-psi mx-psi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@agoallikmaa @tigrannajaryan @mx-psi @pellared