elastic / apm-agent-nodejs

https://www.elastic.co/guide/en/apm/agent/nodejs/current/index.html
BSD 2-Clause "Simplified" License
582 stars 224 forks source link

add `transactionIgnoreUserAgents` config var using wildcard-matcher, deprecate `ignoreUserAgents` #1950

Open trentm opened 3 years ago

trentm commented 3 years ago

From discussion on https://github.com/elastic/apm/issues/395 it was noted that the "ignore_user_agents" config var in the Java agent is the only "dynamic" config var that (a) has the same name as a config var in another language agent, but (b) is not consistent in semantics. To get all dynamic config vars to also be central config vars, those vars need to be consistent across agents. The proposal from discussion is to:

  1. for now mark ignore_user_agents in the Java agent as not dynamic (https://github.com/elastic/apm-agent-java/issues/1632)
  2. plan to change the Node.js Agent's handling of ignoreUserAgents to be consistent (i.e. to use Wildcard Matcher patterns). That is this ticket. This will need to wait for the next major version as it is technically a backward incompatible change. I say "technically" because we don't have data or indication that there is any customer usage of this config var.

proposed change to ignoreUserAgents semantics

Currently ignoreUserAgents takes an array of strings or regexps. If a string, it is "matched against the beginning of the User-Agent."

Proposed changes

An alternative proposal:

Edit: The "alternative proposal" is the winner (see comments below).

eyalkoren commented 3 years ago

@trentm what do you say we go with the "alternative proposal"? It sounds like it would be much simpler to implement in the Node agent, and for the Java agent it is really a matter of minutes to change (including backward compatibility). If we use transaction_ignore_user_agents it would make sense alongside transaction_ignore_urls

trentm commented 3 years ago

@eyalkoren Sounds good to me. Let's do that then.

So, will https://github.com/elastic/apm-agent-java/issues/1632 change then to be "add transaction_ignore_user_agents (same behaviour as ignore_user_agents) and deprecate ignore_user_agents" ?

eyalkoren commented 3 years ago

Sounds good to me. Let's do that then.

Great, then I'll draft it though the spec and ask for your review

So, will elastic/apm-agent-java#1632 change then to be "add transaction_ignore_user_agents (same behaviour as ignore_user_agents) and deprecate ignore_user_agents" ?

@AlexanderWert already did that 🙂

eyalkoren commented 3 years ago

@trentm I added this to the spec in https://github.com/elastic/apm/pull/412. Please review when you get the chance, so we can finalize and implement in the Java agent. Thanks!