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

Make agent.transactionNameNormalizer load config data. #69

Closed
wants to merge 1 commit into from

Conversation

eiriksm
Copy link

@eiriksm eiriksm commented Oct 27, 2013

Howdy, thanks for a great service and module.

I tried to add some config rules to ignore a path in my app, but ended up doubting my regexp skills, since they were not taken into account. So I started debugging, and found that the agent.transactionNameNormalizer does not load from config, thus not loading my ignore rules. This patch fixes that.

I must note that this was just a quick debugging from my side, but surely you guys can quickly see if this was indeed something you overlooked, or if there is something else wrong.

Should probably have a quick test too?

Anyways. Thanks again. Hope this helps.

(btw, sorry for the duplicate. Accidentally committed as my vagrant machine)

@othiym23
Copy link
Contributor

Hey, Eirik, thanks for the pull request! We're not going to accept it, but I'll explain what's going on here, and maybe that will be a good enough substitute. Here's the logic the agent follows when naming transactions:

  1. When a request comes in, and Express or Restify are in play, intercept calls to the router and use the route's name to name the associated transaction.
  2. While the request is in play, the transaction naming API exported from the module can be used to either set a transaction name or overwrite a transaction name pulled from Express. This allows users to override the Express / Restify names if they don't like them.
  3. Once the response is sent and the transaction is being finalized, if there is no transaction name set yet, apply the URL normalization rules to the raw URL path of the transaction. These can be set either by New Relic's support staff or by you, in your configuration or using the naming / ignoring rules calls on the module API. If there is no matching normalization rule, set the name of the transaction to /*. (This backstop is the cornerstone of how the Node module prevents metric explosions – where there are too many metric names for our service to handle – on the server side).
  4. Finally, once the transaction name has been set, apply the transaction name normalization rules. These rules are only set by New Relic. New Relic needs the final say on transaction naming because badly-formed URL normalization rules and transaction-naming calls can cause metric explosions, which will lead to your application being blackholed. Nobody wants that.

I know the interplay between steps 1-3 can be confusing, but our rationale in designing it to work that way is that using the route names is going to be sufficient for the majority of people using Express / Restify, and the naming and ignoring rules are a way for users who are running without a framework to group together large numbers of requests without their metrics getting backstopped to /*. If you want tighter control over naming and you're using Express or Restify, use setTransactionName or setControllerName – the naming API is written in such a way that if you disable the agent via configuration but are still including it, a stub interface is exported that will log calls (at log level of debug or trace) but is otherwise a null operation.

@othiym23 othiym23 closed this Oct 27, 2013
@eiriksm
Copy link
Author

eiriksm commented Oct 28, 2013

Wow. Thanks for a quick and elaborate response. I had the feeling this was not an oversight!

I think I understand the flow of it. However, I still I can not get ignoring to work, though (I realize this is looking more like a support request now, but I hope you bear with me :p)

In the documentation there is a snippet about ignoring socket.io's poll path. It is referenced as this:

// newrelic.js
exports.config = {
  // other configuration
  rules : {
    ignore : [
      '^/socket.io/.*/xhr-polling'
    ]
  }
};

My use-case is very similar. I want to ignore /poll, because it is used for long-polling. I have tried all kinds of combinations, and for what I can find out, it ends up not being ignored because of this (from line 113 in transaction.js):

  // the URL normalizer returns no name if a path should be ignored
  if (this.partialName) {
    var normalizer = this.agent.transactionNameNormalizer
      , normalized = normalizer.normalize(NAMES.WEB + '/' + this.partialName)
      ;

    // the transaction normalizer returns no name if a transaction should be ignored
    if (normalized) {
      this.name = normalized;
      return;
    }
  }

  this.ignore = true;

Since this.agent.transactionNameNormalizer has no this.config.rules, it never gets ignored. If the transactionNameNormalizer is not meant to read config, could it be that this should rather be

var normalizer = this.agent.urlNormalizer

(which also makes ignoring work again)?

Following your answer I can now rename my transactions, but I can not ignore neither the renamed transaction or the "original" transaction.

Am I just doing everything wrong here? Very grateful that you took the time to respond in such a way, and it would be awesome if you could hint me in what direction I am doing something wrong here.

@othiym23
Copy link
Contributor

I think you've hit upon a hole in the transaction-naming API, but first: is your /poll route being handled by Express or Restify right now? That's consistent with what you're saying, so let me provide a little more context (someday I'll run out!):

If you're using Express / Restify, the name for the transaction is being set by the New Relic code that grabs the route names from the router. Since the name is already set by the time the final call to name the transaction is made, the URL normalizer, which includes the ignoring rules, isn't run on the transaction, because the name isn't going to be URL-based. I see why using the transaction name normalizer is appealing there, but New Relic needs to reserve that escape hatch for itself.

What I think needs to be there but isn't right now, is an API call that just says "ignore this transaction". I'll need to talk this over with my team here, but if that's the case, it's an easy addition to the API and may be in a point release soon. If you wanted to try hacking it in for yourself in the meantime, if you look in api.js, you can see how some of the calls grab the current transaction. All you need to do is add a call that looks something like this:

API.prototype.setIgnoreTransaction = function (ignored) {
  var transaction = this.agent.tracer.getTransaction();
  if (!transaction) {
    return logger.warn("No transaction found when trying to ignore one.");
  }

  transaction.ignore = ignored;
};

(method name is kinda gross, but that's what our other language APIs use, so Node will probably follow its lead when I do that for real)

This will also automatically add another call to the stub API that gets exported when New Relic is disabled (via the configuration, obvs you still need to require the module so you can get the API), so you can add setIgnoreTransaction calls to your route handlers without worrying about it incurring overhead (or crashing) when you've disabled New Relic.

If you give that a shot, let me know how it goes!

jsumners-nr pushed a commit to jsumners-nr/node-newrelic that referenced this pull request Apr 11, 2024
upgraded setup-node to v2 and changed linting version to `lts/*`
jsumners-nr pushed a commit to jsumners-nr/node-newrelic that referenced this pull request Apr 16, 2024
bizob2828 pushed a commit to bizob2828/node-newrelic that referenced this pull request Apr 19, 2024
Adds GitHub Actions CI. Removes Travis.
bizob2828 pushed a commit to bizob2828/node-newrelic that referenced this pull request Apr 23, 2024
Adds GitHub Actions CI. Removes Travis.
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.

2 participants