NH-90184: add doc for add_tracer api call #174

xuan-cao-swi · 2025-01-06T15:28:42Z

Description

Test (if applicable)

cheempz

I reran one of the failed 3.1.0 tests and it still failed, could you take a look?

Regarding the README, i think add_tracer belongs in the convenience category, so how about updating that section to talk both about our in_span and our add_tracer methods.

And replace the introduction part To instrument a custom function outside the default scope... (which i think is a given, that if a customer bothers using the API, it's because they'd like to instrument something not covered by autoinsrumentation) with how add_tracer should be used, e.g. does it:

add a custom span to the specified instance or class method
require the target method be already defined?
take optional parameters like examples you gave in NH-90184: add feature - add_tracer on function #153

And we need the docstring in the source code so things show up in RubyDoc.

xuan-cao-swi · 2025-01-07T17:22:52Z

And replace the introduction part To instrument a custom function outside the default scope... (which i think is a given, that if a customer bothers using the API

Updated.

failed 3.1.0 tests

It's caused by old activerecord gem inside the marginalia test case. It's removed so I just remove the gemfile as well (it's done in pg dbo pr).

cheempz

Getting close! Made a few wording changes and one larger change/question on the instance vs class example.

Also, how about the docstring (or are you waiting for this README content to finish revising before adding)?

README.md

cheempz · 2025-01-07T21:06:01Z

README.md

+  # instrument class function create_session
+  def create_session(user)
+  end
+  add_tracer :create_session, 'custom_name', { attributes: { 'foo' => 'bar' }, kind: :consumer }
+
+  # instrument instance function self.create_session
+  def self.create_session(user)
+  end
+
+  class << self


Suggested change

# instrument class function create_session

def create_session(user)

end

add_tracer :create_session, 'custom_name', { attributes: { 'foo' => 'bar' }, kind: :consumer }

# instrument instance function self.create_session

def self.create_session(user)

end

class << self

def create_session(user)

end

def self.create_session(user)

end

# instrument instance method create_session

add_tracer :create_session, 'custom_name', { attributes: { 'foo' => 'bar' }, kind: :consumer }

# instrument class method create_session

class << self

I think create_session is the instance method, while self.create_session is the class method. And is the include SolarWindsAPM::API::Tracer on line 93 needed by the "instrument instance method" usage? It might be clearer if we just show the instrument instance vs class method as two separate snippets.

Do you mean like putting them in separate code block?

Also, how about the docstring

Updated

Yes, i mean two separate code blocks, because currently it's unclear where include SolarWindsAPM::API::Tracer is needed for each scenario--the current example has it once at the start of class definition, then again via the class << self construct. If we keep a single code block example it would be good to clarify why.

Co-authored-by: Lin Lin <[email protected]>

README.md

cheempz · 2025-01-07T23:44:12Z

README.md

+  # instrument class function create_session
+  def create_session(user)
+  end
+  add_tracer :create_session, 'custom_name', { attributes: { 'foo' => 'bar' }, kind: :consumer }
+
+  # instrument instance function self.create_session
+  def self.create_session(user)
+  end
+
+  class << self


Yes, i mean two separate code blocks, because currently it's unclear where include SolarWindsAPM::API::Tracer is needed for each scenario--the current example has it once at the start of class definition, then again via the class << self construct. If we keep a single code block example it would be good to clarify why.

cheempz · 2025-01-07T23:44:55Z

lib/solarwinds_apm/api/custom_instrumentation.rb

@@ -14,6 +14,37 @@ def self.included(base)
      end

      module ClassMethods
+        # Helper function to instrument custom function


Can we be consistent and just use "method" instead of sometimes "function"? Please check for all instances in this docstring.

Updated to use method

cheempz · 2025-01-07T23:45:53Z

lib/solarwinds_apm/api/custom_instrumentation.rb

+        #
+        # * +method_name+ - (String) A non-empty string that match the function name that need to be instrumented
+        # * +span_name+ - (String, optional, default = method_name) A non-empty string that define the span name (default to method_name)
+        # * +options+ - (Hash, optional, default = {}) A hash with desired attributes


and span kind?

span kind is part of options hash. Updated the docstring.

cheempz · 2025-01-07T23:46:29Z

lib/solarwinds_apm/api/custom_instrumentation.rb

+        # * +span_name+ - (String, optional, default = method_name) A non-empty string that define the span name (default to method_name)
+        # * +options+ - (Hash, optional, default = {}) A hash with desired attributes
+        #
+        # === Example:


in this example there is no include SolarWindsAPM::API::Tracer, should there be?

Yes, updated.

Co-authored-by: Lin Lin <[email protected]>

lib/solarwinds_apm/api/custom_instrumentation.rb

cheempz

LGTM, thanks @xuan-cao-swi!

NH-90184: add doc for add_tracer api call

ce85214

xuan-cao-swi marked this pull request as ready for review January 6, 2025 15:28

xuan-cao-swi requested a review from a team as a code owner January 6, 2025 15:28

cheempz requested changes Jan 6, 2025

View reviewed changes

revision and remove rails related gem install

3bd2dff

xuan-cao-swi requested a review from cheempz January 7, 2025 17:23

cheempz reviewed Jan 7, 2025

View reviewed changes

xuan-cao-swi and others added 6 commits January 7, 2025 17:38

Update README.md

ad67739

Co-authored-by: Lin Lin <[email protected]>

Update README.md

3bc43f4

Co-authored-by: Lin Lin <[email protected]>

Update README.md

c972696

Co-authored-by: Lin Lin <[email protected]>

Update README.md

6e3ead2

Co-authored-by: Lin Lin <[email protected]>

Update README.md

9007d40

Co-authored-by: Lin Lin <[email protected]>

add stringdoc

2bdf975

xuan-cao-swi requested a review from cheempz January 7, 2025 22:54

cheempz reviewed Jan 7, 2025

View reviewed changes

xuan-cao-swi and others added 2 commits January 8, 2025 10:09

Update README.md

4ec3ef9

Co-authored-by: Lin Lin <[email protected]>

revision

9fc232b

github-advanced-security bot found potential problems Jan 8, 2025

View reviewed changes

lib/solarwinds_apm/api/custom_instrumentation.rb Fixed Show fixed Hide fixed

lint

c44cfc3

xuan-cao-swi requested a review from cheempz January 8, 2025 17:18

cheempz approved these changes Jan 8, 2025

View reviewed changes

xuan-cao-swi merged commit fbfbffb into main Jan 8, 2025
14 checks passed

xuan-cao-swi deleted the NH-90184 branch January 8, 2025 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NH-90184: add doc for add_tracer api call #174

NH-90184: add doc for add_tracer api call #174

xuan-cao-swi commented Jan 6, 2025

cheempz left a comment •

edited

Loading

xuan-cao-swi commented Jan 7, 2025

cheempz left a comment

cheempz Jan 7, 2025

cheempz Jan 7, 2025

xuan-cao-swi Jan 7, 2025

xuan-cao-swi Jan 7, 2025

cheempz Jan 7, 2025

xuan-cao-swi Jan 8, 2025

cheempz Jan 7, 2025

cheempz Jan 7, 2025

xuan-cao-swi Jan 8, 2025

cheempz Jan 7, 2025

xuan-cao-swi Jan 8, 2025

cheempz Jan 7, 2025

xuan-cao-swi Jan 8, 2025

cheempz left a comment

NH-90184: add doc for add_tracer api call #174

NH-90184: add doc for add_tracer api call #174

Conversation

xuan-cao-swi commented Jan 6, 2025

Description

Test (if applicable)

cheempz left a comment • edited Loading

Choose a reason for hiding this comment

xuan-cao-swi commented Jan 7, 2025

cheempz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheempz left a comment

Choose a reason for hiding this comment

cheempz left a comment •

edited

Loading