microsoft / ApplicationInsights-JS

Microsoft Application Insights SDK for JavaScript
MIT License
646 stars 237 forks source link

Blocking certain URIs/Patterns from fetch tracking (patch included) #1089

Closed ewnavilae closed 4 years ago

ewnavilae commented 4 years ago

I have recently run into an issue where I had to block AppInsights from sending correlation headers to a specific URI via fetch. I understand I can stop these URIs from being tracked with a plugin that prevents this information from being sent to the telemetry server, however this doesn't stop fetch from being hooked and the headers from being sent. I couldn't find a documented feature that results in this behavior.

To work around it, I have applied the following patch using yarn patch-package: https://gist.github.com/ewnavilae/07dd5aa58377db6c9ab6df116467b44f

If enough people value this change I may try to submit a pull request actually implementing this correctly. Is there an already implemented, supported way of doing this?

markwolff commented 4 years ago

Does using config.correlationHeaderExcludedDomains solve this problem?

ewnavilae commented 4 years ago

In my use case, I only need to filter a specific URI from a domain I am otherwise including. I may have tested it wrong but it didn't produce the behavior I required.

markwolff commented 4 years ago

Your patch seems to have similar behavior to the correlationHeaderExcludedDomains. It accepts wildcard *. Can you give an example of your scenario?

ewnavilae commented 4 years ago

In my AppInsights configuration, I tried the following options:

          correlationHeaderDomains: [
           "my_api_server",
           ],
          correlationHeaderExcludedDomains: [
            "my_api_server/AMCalendarEventsAPI/IcsGenerator.ashx",
           ],

This still results in correlation headers being sent to that URI. This is problematic because it results in the following error:

Access to fetch at 'my_api_server/AMCalendarEventsAPI/IcsGenerator.ashx' from origin
'http://localhost:8080' has been blocked by CORS policy: Request header field request-id is not
allowed by Access-Control-Allow-Headers in preflight response.

Am I doing something wrong here?

markwolff commented 4 years ago

I believe you need to include the actual hostnames in the 2 config arrays since we do URL parsing on them during the comparison. e.g. localhost:8080/my_api_server and production.endpoint.com/my_api_server. Perhaps they should be updated to support wild carding of the hostname as well */my_api_server

ewnavilae commented 4 years ago

I'm sorry, I don't understand how that would get correlation headers to be sent to my API server on all URIs except the one I needed to blacklist.

I got this fixed by updating the problematic endpoint to allow the correlation headers.

Still, I believe a better mechanism to choose where to send correlation headers would be useful... Perhaps an option like shouldSendCorrelationHeaders whose signature would be ( url: string ) => boolean? Not sure if it's worth implementing as it would seem this isn't highly requested.

markwolff commented 4 years ago

See the implementation here. The URLs you configure with must include the hostname, and not just the route information.

https://github.com/microsoft/ApplicationInsights-JS/blob/b3d82fe45cdb281abecef92a2313dd79ed977bd7/shared/AppInsightsCommon/src/Util.ts#L672-L712

ewnavilae commented 4 years ago

Reading that implementation, it would appear there is no way to exclude a URL inside a host, since only the host is taken into account in the excludedDomains match. The point was to exclude a single URL.

markwolff commented 4 years ago

Yes it seems to only be looking at the hostname and no route info so I'd consider this shortfall a bug/enhancement 😃

MSNev commented 4 years ago

Release is now fully deployed

github-actions[bot] commented 3 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.