docs: Fix doc for sidekiq options #825
Conversation
kohbis
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
robbkidd,
simi,
kaylareopelle and
xuan-cao-swi
as code owners
| @@ -38,6 +38,10 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base | |||
| # - `:queue` - the span names will be set to '<destination / queue name> <operation>'. | |||
| # - `:job_class` - the span names will be set to '<job class name> <operation>'. | |||
| option :span_naming, default: :queue, validate: %I[job_class queue] | |||
| # @!macro | |||
| # @!method $1 | |||
| # @instrumentation_option_default `:$2` | |||
There was a problem hiding this comment.
Why using :$2 here but in :propagation_style changed from :$2 to $2?
Should line 33 also need to change to $2 if that's the correct format?
There was a problem hiding this comment.
Somehow I thought it would be easier to see the symbols if they are code blocks.
But it's not my strong request, so it's ok that unified them one way or the other 🙏
There was a problem hiding this comment.
Ah, I see, it's for yardoc; my bad. It looks good, thanks
There was a problem hiding this comment.
I don't have any other preference about illustration of boolean and nil value (i.e. could be string, code block or darken like **$2**); unless others suggest difference.
arielvalentin
changed the title
|
Is there anything else I should do in this PR? |
There was a problem hiding this comment.
Even thought I really like this, it turns out my suggestion to add documentation for individual options ended up confusing people.
Sorry about that.
We'll eventually be putting all of the configurations as class level docs to eliminate any confusion.
Thank you again for taking the time to implement my suggestions and sorry for the inconvenience.
|
@arielvalentin So should I move it to the class level documentation like #834 for now? |
|
Yes please. I sincerely apologize for having mislead you the first time around. |
|
@arielvalentin Moved to Class level. Could you please check 🙏 9ea801a 
|
|
@robbkidd may I ask for your review here? |
instrumentation/sidekiq/lib/opentelemetry/instrumentation/sidekiq.rb
Outdated
Show resolved
Hide resolved
instrumentation/sidekiq/lib/opentelemetry/instrumentation/sidekiq/instrumentation.rb
Outdated
Show resolved
Hide resolved
…kiq.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>
…kiq/instrumentation.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>
|
@kaylareopelle Thank you for your review. I've committed suggestions |
There are omissions to correct on #824