auto convert :auto to :prepend/:chain in config #1930

fallwith · 2023-04-07T22:13:19Z

For :'instrumentation.*' configuration parameters that are set to :auto (the default), the agent will automatically determine whether to use method prepending or chaining. The agent will now update its in-memory configuration to give each relevant parameter a value of either :prepend or :chain so that the result of the determination can be introspected. This is intended to help 3rd party libraries that wish to further enhance the agent's instrumentation capabilities by prepending or chaining additional logic. Environment variable, YAML file, and server-side configuration based values are not impacted.

resolves #1924

For `:'instrumentation.*'` configuration parameters that are set to :auto (the default), the agent will automatically determine whether to use method prepending or chaining. The agent will now update its in-memory configuration to give each relevant parameter a value of either :prepend or :chain so that the result of the determination can be introspected. This is intended to help 3rd party libraries that wish to further enhance the agent's instrumentation capabilities by prepending or chaining additional logic. Environment variable, YAML file, and server-side configuration based values are not impacted.

prateeksen · 2023-04-10T11:28:37Z

@fallwith There is a bug in the replace_or_add_config method which updates the config when there is any new config is received from server source or any other source.
evaluate_and_apply_transformations method always updates from Default source and hence the changes made in between(in this case the instrumentation config) are lost once the server source is applied.
For the instrumentation config either we will need to create a new Map or need to fix this issue.
Probable fix can be, update only those parameters in config which are supposed to be updated by server source configuration.

CHANGELOG.md

add missing ** closing style Co-authored-by: Kayla Reopelle (she/her) <[email protected]>

when resetting the configuration manager's cache, do not allow already determined dependency detection params to be reverted back to either `nil` or `'auto'`.

fallwith · 2023-04-20T00:42:35Z

@fallwith There is a bug in the replace_or_add_config method which updates the config when there is any new config is received from server source or any other source. evaluate_and_apply_transformations method always updates from Default source and hence the changes made in between(in this case the instrumentation config) are lost once the server source is applied. For the instrumentation config either we will need to create a new Map or need to fix this issue. Probable fix can be, update only those parameters in config which are supposed to be updated by server source configuration.

Hi @prateek-ap. The dependency detection based parameters can't be configured server-side so we should be okay to simply preserve them after auto-detection. I have updated the cache resetting logic with 7735f92.

for each default source test, make sure the config cache is reset

prateeksen · 2023-04-20T06:03:35Z

Hi @fallwith The cache data is now preserving, Thanks.
But, I have observation that few instrumentation.<module> parameter values are enabled, while the VALID_CONFIG_VALUES are only [:auto, :disabled, :prepend, :chain].
This makes discrepancies for few module instrumentation, example:
:"instrumentation.mongo"=>"enabled"
:"instrumentation.excon"=>"enabled"
How to resolve instrumentation mechanism used(alias or prepend) by Dependency detection for these cases?

(for the sake of older Rubies) don't attempt to reference `@cache`, even for an evaluation of truthiness, without making sure it is defined first

fallwith · 2023-04-21T01:20:34Z

Hi @fallwith The cache data is now preserving, Thanks. But, I have observation that few instrumentation.<module> parameter values are enabled, while the VALID_CONFIG_VALUES are only [:auto, :disabled, :prepend, :chain]. This makes discrepancies for few module instrumentation, example: :"instrumentation.mongo"=>"enabled" :"instrumentation.excon"=>"enabled" How to resolve instrumentation mechanism used(alias or prepend) by Dependency detection for these cases?

@prateek-ap this is a great point! The Mongo and Excon instrumentation config parameters are definitely odd. As you point out, they are not set to auto|disabled|prepend|chain like the others, but enabled|disabled instead. ~~And I think there might also be a bug preventing them from respecting disabled. I will use a separate PR to address that issue.~~ [UPDATE: nope, not an issue]

The good news is that you don't not have to worry about the newrelic_rpm Mongo and Excon instrumentations because they do not touch any module code. The Mongo instrumentation calls Mongo::Monitoring::Global.subscribe and the Excon instrumentation calls << on Excon.defaults[:middlewares]. Because the newrelic_rpm agent is not performing any module prepending or chaining for the Mongo and Excon libraries, you won't have to worry about matching the same approach. You can either follow the same patterns used with subscribe and << or perform module prepending/chaining if you prefer. And optionally you can skip the instrumentation entirely if you find the agent's config value is set to 'disabled'.

prateeksen · 2023-04-21T02:52:52Z

Hi @fallwith I understand that APM doesn't doing alias or prepend on mongo and excon library and that's why, by default have enabled, but In case of Security agent I will need a deterministic decision, otherwise, I will instrument using default approach (i.e. prepend) and that can conflict with other library instrumenting mongo or excon may be using chaining.

hannahramadan · 2023-04-21T15:39:43Z

lib/new_relic/dependency_detection.rb

+      if config_key && auto_configured?
+        NewRelic::Agent.config.instance_variable_get(:@cache)[config_key] = use_prepend ? :prepend : :chain
+      end
+      use_prepend


Is this saying we are always returning true or false?

Precisely. We hook into this existing method that returns a boolean, so here we make sure we still always return a boolean.

Got it I see what is happening now. Thank you :)

'becames' -> 'becomes' comment spelling fix

github-actions · 2023-04-21T16:19:55Z

SimpleCov Report

	Coverage	Threshold
Line	93.97%	93%
Branch	85.47%	85%

kaylareopelle

This is great as-is, take or leave the CHANGELOG rec.

kaylareopelle · 2023-04-21T15:37:43Z

CHANGELOG.md

@@ -2,6 +2,10 @@

 ## dev

+- **Feature: The agent configuration will now reflect whether method prepending or chaining was used per instrumentation**


Would you be up for changing "method prepending or chaining" to "module prepending or method chaining" in the title and the body copy?

Certainly! Addressed with bf3b5ee

fallwith · 2023-04-21T19:24:12Z

Hi @fallwith I understand that APM doesn't doing alias or prepend on mongo and excon library and that's why, by default have enabled, but In case of Security agent I will need a deterministic decision, otherwise, I will instrument using default approach (i.e. prepend) and that can conflict with other library instrumenting mongo or excon may be using chaining.

Hi @prateek-ap. With this PR/branch, for all config parameters that start with instrumentation., you can rely on the following for their values:

prepend: Agent used the prepend approach to provide instrumentation
chain: Agent used the chain approach to provide instrumentation
disabled: Agent did not provide instrumentation
enabled: Agent provided instrumentation, but did not perform monkeypatching (no chain, no prepend)

So if you see 'enabled', feel free to use a default desired approach (prepend) without any worry of conflicting. For all instrumentation. parameters that have a value of 'enabled', the agent used neither prepend nor chain, so there won't be a conflict.

For Mongo and Excon in particular, you are welcome to follow the same pattern that we use for working with those libraries (for Mongo we subscribe, and for Excon we add to the middleware array) or if you prefer you can use prepend. Either way, you should not conflict with the agent for those two.

Update PR 1930's CHANGELOG entry to use the correct "module prepending" and "method chaining" verbiage. Also replace the potentially confusing "per" with "for".

fallwith · 2023-04-21T20:04:05Z

@prateek-ap these changes are in the dev branch now.

With [1930](#1930) the agent will now automatically convert any configured instrumentation value of `:auto` into either `:prepend` or `:chain` once the determination of which approach to take has been dynamically determined. This will allow the agent's config to be inspected after the `:prepend` / `:chain` determination is made to see what `:auto` actually becomes under the hood. But 1930 left an edge case gap open in the form of not allowing the configuration to convey instrumentation with unsatisfied dependencies. If a config value is set to `:prepend` or `:chain` in the configuration (overriding the default `:auto` value) and a dependency is unsatisfied (the library to instrument is not found on the system, it is of an unsupported version, etc.) then the value would end up staying as `:prepend` or `:chain` and incorrectly convey to anyone inspecting the config that a certain method of instrumentation was used when in actuality no instrumentation was performed at all. Now, if an instrumentation's dependencies are unsatisfied, the corresponding configuration value will be updated with a value of `:unsatisfied`.

kaylareopelle reviewed Apr 10, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

abd5c89

add missing ** closing style Co-authored-by: Kayla Reopelle (she/her) <[email protected]>

fallwith requested review from hannahramadan and tannalynn as code owners April 19, 2023 22:56

config manager: don't clobber dependency detection

7735f92

when resetting the configuration manager's cache, do not allow already determined dependency detection params to be reverted back to either `nil` or `'auto'`.

default source test: flush config cache on setup

0d6eb27

for each default source test, make sure the config cache is reset

manager: check for @cache

24f8163

(for the sake of older Rubies) don't attempt to reference `@cache`, even for an evaluation of truthiness, without making sure it is defined first

hannahramadan reviewed Apr 21, 2023

View reviewed changes

'becames' -> 'becomes' typo fix

5084f2f

'becames' -> 'becomes' comment spelling fix

kaylareopelle previously approved these changes Apr 21, 2023

View reviewed changes

hannahramadan previously approved these changes Apr 21, 2023

View reviewed changes

PR 1930 CHANGELOG updates

bf3b5ee

Update PR 1930's CHANGELOG entry to use the correct "module prepending" and "method chaining" verbiage. Also replace the potentially confusing "per" with "for".

fallwith dismissed stale reviews from hannahramadan and kaylareopelle via bf3b5ee April 21, 2023 19:28

hannahramadan approved these changes Apr 21, 2023

View reviewed changes

fallwith merged commit 4bec6de into dev Apr 21, 2023

fallwith deleted the eritrea_okra branch April 21, 2023 20:02

fallwith mentioned this pull request May 10, 2023

Absent instrumented libraries should be identified as such #2004

Closed

fallwith mentioned this pull request May 13, 2023

instrumentation config: use :unsatisfied #2011

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auto convert :auto to :prepend/:chain in config #1930

auto convert :auto to :prepend/:chain in config #1930

fallwith commented Apr 7, 2023

prateeksen commented Apr 10, 2023

fallwith commented Apr 20, 2023

prateeksen commented Apr 20, 2023

fallwith commented Apr 21, 2023 •

edited

Loading

prateeksen commented Apr 21, 2023

hannahramadan Apr 21, 2023

fallwith Apr 21, 2023

hannahramadan Apr 21, 2023

github-actions bot commented Apr 21, 2023

kaylareopelle left a comment

kaylareopelle Apr 21, 2023

fallwith Apr 21, 2023

fallwith commented Apr 21, 2023

fallwith commented Apr 21, 2023

		@@ -2,6 +2,10 @@

		## dev

		- Feature: The agent configuration will now reflect whether method prepending or chaining was used per instrumentation

auto convert :auto to :prepend/:chain in config #1930

auto convert :auto to :prepend/:chain in config #1930

Conversation

fallwith commented Apr 7, 2023

prateeksen commented Apr 10, 2023

fallwith commented Apr 20, 2023

prateeksen commented Apr 20, 2023

fallwith commented Apr 21, 2023 • edited Loading

prateeksen commented Apr 21, 2023

hannahramadan Apr 21, 2023

Choose a reason for hiding this comment

fallwith Apr 21, 2023

Choose a reason for hiding this comment

hannahramadan Apr 21, 2023

Choose a reason for hiding this comment

github-actions bot commented Apr 21, 2023

SimpleCov Report

kaylareopelle left a comment

Choose a reason for hiding this comment

kaylareopelle Apr 21, 2023

Choose a reason for hiding this comment

fallwith Apr 21, 2023

Choose a reason for hiding this comment

fallwith commented Apr 21, 2023

fallwith commented Apr 21, 2023

fallwith commented Apr 21, 2023 •

edited

Loading