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

Add TransactionIgnoreUrls setting #904

Merged
merged 9 commits into from
Jul 21, 2020
Merged

Add TransactionIgnoreUrls setting #904

merged 9 commits into from
Jul 21, 2020

Conversation

gregkalapos
Copy link
Contributor

@gregkalapos gregkalapos commented Jul 15, 2020

Solves #688, implements elastic/apm#144

The TransactionIgnoreUrls config takes a list and matches each url path against it. If the given url path is in the list, then it'll be ignored - meaning no transaction will be captured.

It checks the path part of the url. E.g. adding /home/index will match all of the followings:

  • https://www.myCoolSite.com/home/index
  • http://localhost/home/index
  • http://whatever.com/home/index?value1=123 (so query string is ignored for matching)

By default it's case insensitive.

ToDo:

  • Apply this also in ASP.NET Classic
  • More tests
  • Think about custom transactions and how to apply it there

@apmmachine
Copy link
Contributor

apmmachine commented Jul 15, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Started by user Gergely Kalapos, Restarted from build #11, stage Initializing]

  • Start Time: 2020-07-21T14:02:51.155+0000

  • Duration: 53 min 46 sec

Test stats 🧪

Test Results
Failed 0
Passed 13571
Skipped 0
Total 13571

Steps errors

Expand to view the steps failures

  • Name: Build
    • Description: .ci/windows/dotnet.bat

    • Duration: 0 min 23 sec

    • Start Time: 2020-07-21T14:15:07.782+0000

    • log

@gregkalapos gregkalapos marked this pull request as ready for review July 16, 2020 16:19
@gregkalapos gregkalapos requested a review from bmorelli25 July 16, 2020 16:19
Used to restrict requests to certain URLs from being instrumented.

This property should be set to a list containing one or more strings.
When an incoming HTTP request is detected, its request path will be tested against each element in this list.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bmorelli25 I think this part could be improved a lot. In the issue description I write about what part of the URL is matched, but I think this text does not make it clear.

Can you come up with something better? I'd like to have something that describes very well what part of the URL is exactly matched - I think most docs just call this part 'path`, but I'm not sure its meaning is generally known.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know, I actually don't find this difficult to understand. With that being said, an example here certainly wouldn't hurt and would go a long way to making this feature easier to understand. What about adding what you have in the description to these docs?

Used to ...

This property should be set to a list containing one or more strings.
When an incoming HTTP request is detected, its request path will be tested against each element in this list.
For example, adding `/home/index` to this list would match and remove instrumentation from the following URLs:

[source,txt]
----
https://www.mycoolsite.com/home/index
http://localhost/home/index
http://whatever.com/home/index?value1=123
----

This option supports ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great; I also think adding an example helps. I added it. Thanks!

Copy link
Member

@bmorelli25 bmorelli25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs LGTM

@gregkalapos gregkalapos merged commit 6ce1020 into elastic:master Jul 21, 2020
@gregkalapos gregkalapos deleted the TransactionIgnoreUrls branch July 21, 2020 14:58
@arivasvera
Copy link

Hi @gregkalapos ,

Is this feature working?

I'm using Elastic.Apm.NetCoreAll version 1.6.1 and in my appsettings.json I have this:

"ElasticApm": {
    "SecretToken": "MY_TOKEN",
    "ServerUrls": "https://example.com",
    "ServiceName": "my-app",
    "TransactionIgnoreUrls": ["/api/healthcheck"]
  }

I have my Startup like this:

using Elastic.Apm.AspNetCore;
using Elastic.Apm.DiagnosticSource;
using Elastic.Apm.EntityFrameworkCore;

public class Startup
{
  public void Configure(IApplicationBuilder app, IHostingEnvironment env)
  {
    app.UseElasticApm(Configuration, new HttpDiagnosticsSubscriber(), new EfCoreDiagnosticsSubscriber());
    //…rest of the method
  }
  //…rest of the class
}

but I'm still getting transactions when http://mysite.com/api/healthcheck is requested

am I doing something wrong?

Thanks in advance

@bmorelli25
Copy link
Member

@arivasvera, I don't think this commit made it into version 1.6.1. It looks like it was merged a day after the release.

@arivasvera
Copy link

Thank you for your response @bmorelli25

I hope this feature will release soon :)

Best regards

@arivasvera
Copy link

hello again,

do you guys have an estimated date to release this feature?

again, thanks in advance

@PBarnard
Copy link

PBarnard commented Dec 12, 2020

@bmorelli25 Has this been released yet? I don't see it anywhere on the releases page - https://github.com/elastic/apm-agent-dotnet/releases

@gregkalapos
Copy link
Contributor Author

Yes, this was part of the 1.7.0 release - so this setting is out and can be used.

This doc page has all the settings from the latest release.

Regarding questions to release date: We don't publicly communicate specific release dates. Simply because we believe it'd cause more harm than it'd help people. So we'll never be able to answer questions around release date, sorry about that.

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.

5 participants