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 README for Sidekiq instrumentation #386

Merged

Conversation

AzfaarQureshi
Copy link

This PR adds documentation for OpenTelemetry's Sidekiq Instrumentation, closing #241 . The README describes the Sidekiq gem, installation methods and its license, following the pattern set in instrumentation/sinatra and instrumentation/concurrent_ruby for consistency.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 16, 2020

CLA Check
The committers are authorized under a signed CLA.

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

👋 Welcome and thanks for the contribution. This looks good, but I'm wondering if we should point people towards the example (in the examples) folder. WDYT?

@AzfaarQureshi
Copy link
Author

@mwear Good idea! I added a small Examples section. Is this along the lines of what you were thinking?

```
## Examples

Example usage can be seen in the `./example/sidekiq.rb` file [here](https:/open-telemetry/opentelemetry-ruby/blob/master/instrumentation/sidekiq/example/sidekiq.rb)
Copy link
Author

Choose a reason for hiding this comment

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

@mwear is it okay to add a link to the actual file here or is that considered bad practice considering it might become dead if there is ever a code restructure?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's ok fwiw, will defer to @mwear on any formatting prefs. nice work here @AzfaarQureshi

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 this is fine - if we restructure the code, we'll also fix the documentation.

Copy link
Contributor

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

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

very minor formatting changes but otherwise lgtm, nice work @AzfaarQureshi

Copy link
Contributor

@fbogsany fbogsany left a comment

Choose a reason for hiding this comment

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

LGTM after @ericmustin's change. Thanks @AzfaarQureshi!

@AzfaarQureshi
Copy link
Author

@ericmustin oops cant believe I missed the dupe. Thanks for the catch! Its fixed and ready for re-review 😄

Copy link
Contributor

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

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

:shipit: 👍

@fbogsany
Copy link
Contributor

Can you please rebase @AzfaarQureshi ?

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

Thanks @AzfaarQureshi!

@AzfaarQureshi AzfaarQureshi force-pushed the 241-add-documentation-for-sidekiq branch from fff7eaf to ba9363f Compare September 17, 2020 20:29
@fbogsany fbogsany merged commit f3f9ac2 into open-telemetry:master Sep 17, 2020
@fbogsany fbogsany linked an issue Sep 18, 2020 that may be closed by this pull request
Comment on lines +47 to +48
[bundler-home]: https://bundler.io
[bundler-home]: https://bundler.io
Copy link
Contributor

Choose a reason for hiding this comment

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

bundler-home listed twice. Everything looks great.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching that - it was a problem in another PR too, but I was lulled into ignorance after reviewing a few that didn't have that problem.

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.

Add documentation for Sidekiq
6 participants