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

Adding a "HOW-TO: Building your own collector" tutorial to the collector #1277

Merged

Conversation

rquedas
Copy link
Member

@rquedas rquedas commented Apr 5, 2022

Hoping to start a series of "HOW-TO" docs in the collector to help developers learning how to build components like the one I am proposing here Tutorial on how to build a trace receiver #1265

This would be the step one in the journey for component developers.


Preview: https://deploy-preview-1277--opentelemetry.netlify.app/docs/collector/howto-custom-collector/

…tor docs to support the development of a How-to doc series for component development
@rquedas rquedas requested a review from a team April 5, 2022 16:29
@jpkrohling
Copy link
Member

Similar to the other PR, just wanted to mention that you might want to compare this with the blog post I published some time ago, and which is certainly outdated by now:

https://medium.com/opentelemetry/building-your-own-opentelemetry-collector-distribution-42337e994b63

@rquedas
Copy link
Member Author

rquedas commented Apr 5, 2022

Absolutely! Your blog is a more complete walkthrough, this one is using the builder tool so people can quickly bootstrap for component development. I would say they are actually complimentary.

Copy link
Member

@austinlparker austinlparker left a comment

Choose a reason for hiding this comment

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

Some grammar nits

content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
rquedas and others added 5 commits April 8, 2022 10:10
Co-authored-by: Austin Parker <austin@ap2.io>
Co-authored-by: Austin Parker <austin@ap2.io>
Co-authored-by: Austin Parker <austin@ap2.io>
Co-authored-by: Austin Parker <austin@ap2.io>
Co-authored-by: Austin Parker <austin@ap2.io>
@rquedas
Copy link
Member Author

rquedas commented Apr 8, 2022

thank you @austinlparker ! I merged all your suggestions.

Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

Added a few recommendations

content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
@rquedas
Copy link
Member Author

rquedas commented Apr 18, 2022

@austinlparker and @svrnm, can you take a look at the latest update and tell me if there is anything else we need to address?
Thanks!

@austinlparker
Copy link
Member

@svrnm lmk if this is g2g

content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
rquedas and others added 2 commits April 18, 2022 16:28
Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de>
Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de>
@svrnm
Copy link
Member

svrnm commented Apr 19, 2022

@austinlparker , all my issues have been addressed, leaving it to the expert @jpkrohling with the remaining open issues :-)

@austinlparker
Copy link
Member

@jpkrohling is this good to go?

@jpkrohling
Copy link
Member

I'm still not 100% happy with this. @rquedas, is this OK if I send another commit to your PR?

@rquedas
Copy link
Member Author

rquedas commented May 3, 2022

I'm still not 100% happy with this. @rquedas, is this OK if I send another commit to your PR?

Absolutely!

@rquedas
Copy link
Member Author

rquedas commented May 10, 2022

@jpkrohling, I will start refactoring the trace receiver tutorial to support 0.50.0 model changes, so when you are finished with this one we can have both available. Does that sound like a plan?

@cartermp
Copy link
Contributor

@jpkrohling would you still prefer to commit against this branch and then merge, or merge now and contribute a change later?

@rquedas
Copy link
Member Author

rquedas commented Jun 13, 2022

@austinlparker, @svrnm and @jpkrohling, just merged changes to reflect "v0.53.0".
Please review and let me know if there is anything you would like me to change.

Copy link
Member

@austinlparker austinlparker left a comment

Choose a reason for hiding this comment

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

Added several suggestions, but otherwise lgtm. As a general tip, you don't need to add qualifiers to statements ('where you basically pass' vs. 'where you pass') in docs.

content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/howto-custom-collector.md Outdated Show resolved Hide resolved
@rquedas
Copy link
Member Author

rquedas commented Jun 17, 2022

@austinlparker, thank you for taking the time in reviewing it and making the suggestions. I merged all of them.
If you think we are ready, we should also update the index to make the tutorial visible

@austinlparker
Copy link
Member

Docs automatically get added to the sidebar when they're added. @jpkrohling you have a requested change; Would you like to commit that directly or add a PR after the merge?

@chalin chalin force-pushed the doc-collector-howto-custom-collector branch from 951db35 to d478359 Compare June 28, 2022 20:25
@chalin
Copy link
Contributor

chalin commented Jun 28, 2022

Rebased since this PR has been open for a while and needed to catch up.

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

See inline comments

content/en/docs/collector/custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/custom-collector.md Outdated Show resolved Hide resolved
content/en/docs/collector/custom-collector.md Outdated Show resolved Hide resolved
@chalin chalin self-assigned this Jun 29, 2022
@chalin
Copy link
Contributor

chalin commented Jun 29, 2022

@rquedas - see my inline comments for possible final changes
@austinlparker - you had requested changes; are you ready to approve now?

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Made it! Woohoo! Thanks all!

@chalin chalin merged commit b8b4519 into open-telemetry:main Jun 30, 2022
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.

6 participants