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

Add Instana exporter overall structure #13452

Merged
merged 8 commits into from
Aug 24, 2022
Merged

Add Instana exporter overall structure #13452

merged 8 commits into from
Aug 24, 2022

Conversation

hickeyma
Copy link
Contributor

@hickeyma hickeyma commented Aug 22, 2022 •
edited
Loading

Signed-off-by: Martin Hickey martin.hickey@ie.ibm.com
Co-authored-by: Martin Hickey martin.hickey@ie.ibm.com
Co-authored-by: Cedric Ziel cedric@cedric-ziel.com
Co-authored-by: Willian Carvalho willianc@pm.me

Description:

Adding new component, Instana exporter.

This is the first of 3 PRs which adds the overall skeleton structure.

Link to tracking Issue:

Partial #13395

Testing:

Unit Tests

Documentation:

Readme explaining the exporter and its configuration.

@hickeyma hickeyma requested review from a team and Aneurysm9 August 22, 2022 15:39
@hickeyma hickeyma force-pushed the feat/add-instana-exporter-skelton branch from d347350 to 0c2fcb9 Compare August 22, 2022 15:42
@hickeyma
Copy link
Contributor Author

/assign @jpkrohling

@hickeyma
Copy link
Contributor Author

/cc @jpkrohling

@jpkrohling jpkrohling assigned jpkrohling and unassigned codeboten Aug 22, 2022
@jpkrohling jpkrohling requested review from jpkrohling and removed request for Aneurysm9 August 22, 2022 16:14
jpkrohling
jpkrohling reviewed Aug 22, 2022
View reviewed changes
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

This PR seems to be missing a few things from the skeleton, like the Makefile.

exporter/instanaexporter/README.md Outdated Show resolved Hide resolved
exporter/instanaexporter/config/config.go Outdated Show resolved Hide resolved
exporter/instanaexporter/factory.go Outdated Show resolved Hide resolved
exporter/instanaexporter/factory.go Outdated Show resolved Hide resolved
exporter/instanaexporter/factory.go Outdated Show resolved Hide resolved
@hickeyma hickeyma force-pushed the feat/add-instana-exporter-skelton branch from 0c2fcb9 to e996dae Compare August 22, 2022 16:28
@hickeyma hickeyma force-pushed the feat/add-instana-exporter-skelton branch from fa7ea87 to b9211a2 Compare August 22, 2022 17:15
@hickeyma
Copy link
Contributor Author

@jpkrohling Thanks for the review and feedback.

I have updated your comments and it is ready for review again.

This PR seems to be missing a few things from the skeleton, like the Makefile

I am following the When adding a new component and for 1st PR it suggests the: "Readme, configuration, and factory implementation". The Makefile etc. can be added in 2nd PR with the implementation, so it can build.

@hickeyma hickeyma requested a review from jpkrohling August 22, 2022 17:19
@jpkrohling
Copy link
Member

The Makefile etc. can be added in 2nd PR with the implementation, so it can build.

Interesting, so far, our interpretation has been that the Makefile has to be part of the first PR so that it's incorporated into the main build. This way, we can ensure the files are syntactically correct, like the config.go. I believe the only thing you'd have to change in this PR is remove the reference to the non-existing function from the factory.

@hickeyma hickeyma force-pushed the feat/add-instana-exporter-skelton branch from a9283df to 5a62110 Compare August 23, 2022 09:35
@hickeyma
Copy link
Contributor Author

Thanks @jpkrohling for the feedback. I have added the Makefile as you suggested. Let me know if it needs any other changes.

@hickeyma hickeyma force-pushed the feat/add-instana-exporter-skelton branch 2 times, most recently from be67318 to 14aef6b Compare August 23, 2022 14:36
jpkrohling
jpkrohling approved these changes Aug 23, 2022
View reviewed changes
.github/CODEOWNERS Outdated Show resolved Hide resolved
exporter/instanaexporter/README.md Show resolved Hide resolved
exporter/instanaexporter/factory.go Outdated Show resolved Hide resolved
@hickeyma hickeyma force-pushed the feat/add-instana-exporter-skelton branch from 73e0cdb to 52f8c8f Compare August 23, 2022 17:08
@hickeyma
Copy link
Contributor Author

@jpkrohling Thanks for the reviews and feedback. I have updated it and it should be good for review again.

@hickeyma hickeyma requested a review from jpkrohling August 23, 2022 17:27
@hickeyma hickeyma force-pushed the feat/add-instana-exporter-skelton branch from b4cf3fa to d09395b Compare August 23, 2022 18:03
hickeyma and others added 8 commits August 24, 2022 12:57
@hickeyma @cedricziel @willianpc
Add Instana exporter overall structure
38ac637 
Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Co-authored-by: Martin Hickey <martin.hickey@ie.ibm.com>
Co-authored-by: Cedric Ziel <cedric@cedric-ziel.com>
Co-authored-by: Willian Carvalho <willianc@pm.me>
@hickeyma
Update after reviews
b8b3b24 
Review comments:
- open-telemetry#13452 (review)

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
@hickeyma
Update after review
46c224b 
Review comments:
- open-telemetry#13452 (comment)

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
@hickeyma
Integrate exporter into the components
db42e31 
Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
@hickeyma
Update changelog
f53acea 
Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
@hickeyma
Add porto changes
73e6522 
Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
@hickeyma
unit tests
acb607b 
Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
@hickeyma
Update after review
8adc18c 
Review comments:
- open-telemetry#13452 (comment)
- open-telemetry#13452 (comment)

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
@hickeyma hickeyma force-pushed the feat/add-instana-exporter-skelton branch from d09395b to 8adc18c Compare August 24, 2022 11:57
@jpkrohling jpkrohling merged commit 3d3f291 into open-telemetry:main Aug 24, 2022
@hickeyma hickeyma deleted the feat/add-instana-exporter-skelton branch August 24, 2022 16:05
@hickeyma
Copy link
Contributor Author

hickeyma commented Aug 24, 2022 •
edited
Loading

Thanks @jpkrohling for reviews, feedback and merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpkrohling jpkrohling jpkrohling approved these changes

Assignees

@jpkrohling jpkrohling

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hickeyma @jpkrohling @codeboten