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

[BUG] using EndPointUrl (and IngestionEndpoint) results in Telemetry sent to incorrect urls #2197

Closed
sander1095 opened this issue Nov 10, 2023 · 12 comments

Comments

@sander1095
Copy link

sander1095 commented Nov 10, 2023

Description/Screenshot

IMPORTANT: I created this issue with the assumption that my issues lay elsewhere. The comments ended up helping find the root cause of this issue and uncovered the bug. The title of this issue has been updated to reflect this, but the body of this issue has not.


I'm using application insights with an instrumentationkey currently. My configuration looks like this:

 this._applicationInsights = new ApplicationInsights({
      config: {
        instrumentationKey: '%TEMP%', // Will be replaced in the BFF
        disableInstrumentationKeyValidation: true,
        enableAutoRouteTracking: true,
        loggingLevelTelemetry: SeverityLevel.Information,
        endpointUrl: 'track', // Send telemetry to the BFF as a reverse proxy so we don't need to expose the INSTRUMENTATION KEY to the front-end.
      },
    });
    this._applicationInsights.loadAppInsights();

The disableInstrumentationKeyValidation is extremely important for me. I forward all app insights to my reverse proxy which then replaces the %temp% value with the real one. This way I don't need to expose it in the client, which is far more secure❗

I'm currently migrating to the new connectionString property with the same %TEMP value, but there is no disableConnectionStringValidation property available. This causes application insights to load incorrectly...

I need to be able to disable connectionstring validation to keep this process working.

I would really appreciate a fix or workaround!

Steps to Reproduce

  • OS/Browser: Firefox
  • SDK Version [e.g. 22]: ^3.0.5
  • How you initialized the SDK: See code above

Expected behavior
I want to be able to disable connectionstring validation.

Additional context

@MSNev
Copy link
Collaborator

MSNev commented Nov 10, 2023

There is no need for any disableConnectionStringValidation, you can still use the disableInstrumentationKeyValidation (as long as the connection string is well formed)

This is because when we parse the connectionString we populate the normal instrumentationKey property for you from the connectionString and from that point on the validation occurs on the instrumentationKey as normal.

@sander1095
Copy link
Author

as long as the connectionstring is well formed

Well, it isn't in my case ;)

Regardless of that, though. InstrumentationKey support is going to be deprecated and removed in the future, probably including the disableInstrumentationKeyValidation. So I do still think we need to be able to disable validation of the new connectionstring?

@MSNev
Copy link
Collaborator

MSNev commented Nov 13, 2023

Not exactly, while "InstrumentationKey" support is going away, this is just for directly setting the instrumentationKey, the actual InstrumentationKey while still exist, just encoded with the connectionString (like it is today) and it will still require validation.

So I do still think we need to be able to disable validation of the new connectionstring?

No, we will (and are planning) to keep the existing disableInstrumentationKeyValidation for handling the validation of both the instrumentationKey (as per today) and for when only the connectionString (with the instrumentation key included -- also as per today).

@sander1095
Copy link
Author

Thanks for your timely response, it's immensely appreciated!

I will attempt your suggestions tomorrow and report back if it works.

As far as I can remember, setting connectionString to a value and disableInstrumentationKey to true caused errors, but maybe I just remember that wrong.

@sander1095
Copy link
Author

One step forwards, one step back!

I've gotten my application to load when I change the connectionString to a more real-looking value:

this._applicationInsights = new ApplicationInsights({
  config: {
    connectionString: 'InstrumentationKey=xxxx;', // Will be replaced in the BFF
    disableInstrumentationKeyValidation: true,
    enableAutoRouteTracking: true,
    loggingLevelTelemetry: SeverityLevel.Information,
    endpointUrl: 'track', // Send telemetry to the BFF as a reverse proxy so we don't need to expose the INSTRUMENTATION KEY to the front-end
  },
});
this._applicationInsights.loadAppInsights();

However, the telemetry is now sent to https://dc.services.visualstudio.com/v2/track instead of track on my own domain, which was normally done by setting the endpointUrl: afbeelding

Before (Red is my own domain):
afbeelding
After (Blocked by CSP because the browser would never communicate with App Insights servers directly)

afbeelding

This is the most important part, because being able to send it to my own domain means I can insert the real connectionstring there before forwarding it myself to https://dc.services.visualstudio.com/v2/track.

I think this might have been an (undocumented) breaking change.

I used to use "@microsoft/applicationinsights-web": "^2.7.1" and I now switched to "@microsoft/applicationinsights-web": "^3.0.5".

If this is a bug in the SDK, I can simply deal with it by using ^2.7.1 for now and migrate to ^3.0.x when this is fixed. Otherwise I would like to get some guidance how to get this to work in the new SDK version.

Thanks!

@MSNev
Copy link
Collaborator

MSNev commented Nov 14, 2023

If your connection string includes the ingestionendpoint, then this will replace any endpointUrl you provide in your configuration. Just removing that from the connection string you provide should work.

if (_config.connectionString) {
const cs = parseConnectionString(_config.connectionString);
const ingest = cs.ingestionendpoint;
_config.endpointUrl = ingest ? (ingest + DEFAULT_BREEZE_PATH) : _config.endpointUrl; // only add /v2/track when from connectionstring
_config.instrumentationKey = cs.instrumentationkey || _config.instrumentationKey;
}

@MSNev
Copy link
Collaborator

MSNev commented Nov 14, 2023

All "keys" from the parsed connection string become lowercase -- so the one you provide doesn't have to have it all in lowercase.

@sander1095
Copy link
Author

I have found a solution, but it goes against your advice, therefore I'm posting this comment.

The connectionstring I provided in my latest comment was:

InstrumentationKey=xxxx;

If I understand you correctly, this means that ingestionEndpoint is null and that my endpointUrl should be used, making my use-case work. The code that you linked to agrees with this:

const ingest = cs.ingestionendpoint;
_config.endpointUrl = ingest ? (ingest + DEFAULT_BREEZE_PATH) : _config.endpointUrl; // only add /v2/track when from connectionstring

I fully understand what you're saying here, and I hope we can agree on the things said above. I'm grateful for your patience.

The issue

However, as you can see in my latest comment, this is not the case because it still overrides the endpointUrl with the https://dc.services.visualstudio.com/v2/track url, so endpointUrl isn't used.

I looked a bit further into the parseConnectionString function and found the following:

if (result.endpointsuffix) {
// use endpoint suffix where overrides are not provided
const locationPrefix = result.location ? result.location + "." : "";
result.ingestionendpoint = result.ingestionendpoint || ("https://" + locationPrefix + "dc." + result.endpointsuffix);
}
// apply the default endpoints
result.ingestionendpoint = result.ingestionendpoint || DEFAULT_BREEZE_ENDPOINT;

The way I interpret this is that, whilst parsing the connectionstring, the ingestionendpoint is null, so it will be given the DEFAULT_BREEZE_ENDPOINT value, which means that ingestionendpoint can never be null, meaning that endpointUrl will never be used (in the code you linked to above):

const ingest = cs.ingestionendpoint;
_config.endpointUrl = ingest ? (ingest + DEFAULT_BREEZE_PATH) : _config.endpointUrl; // only add /v2/track when from connectionstring

Workaround

I can work around this by using the following connectionstring.

'instrumentationkey=xxxx;ingestionendpoint=/'
(app insights config) endpointUrl: 'track'

When running on https://localhost:5012, this will send all requests to https://localhost:5012/track again. However, I do not really understand why this works. If I read the code you linked, it should now append the DEFAULT_BREEZE_PATH, making the final url https://localhost:5012/v2/track. But apparently it DOES use my endpointUrl:

const ingest = cs.ingestionendpoint;
_config.endpointUrl = ingest ? (ingest + DEFAULT_BREEZE_PATH) : _config.endpointUrl; // only add /v2/track when from connectionstring

What's going on here?

Extra

I do not understand how "lowercase" plays a role here, as this is the exact spelling as showcased in the Azure Portal.

In fact, changing my working example (all lowercase) to this:

InstrumentationKey=xxxx;IngestionEndpoint=/

results in a working code sample, so I don't understand why the lowercase is required here.

@MSNev
Copy link
Collaborator

MSNev commented Nov 14, 2023

Well bother!

Lets tag this issue as a bug so that we can "update" the parseConnectionString to "pass-in" the "user default endpointUrl", this will allow the caller (the AISku.ts) to pass the config value of this default value.

so it would become something like

const cs = parseConnectionString(_config.connectionString, _config.endpointUrl); 

and then in the parse this bit can use that

export function parseConnectionString(connectionString?: string, userDefaultEndpointUrl?: string): ConnectionString {
...
        // apply the default endpoints
        result.ingestionendpoint = result.ingestionendpoint || userDefaultEndpointUrl || DEFAULT_BREEZE_ENDPOINT;

@sander1095
Copy link
Author

Seems like a great fix! Perhaps it would be worth creating a new issue for this, as the current issue is kind of all over the place.

Also, I'm just wondering if this needs to be applied to other App Insights SDK's as well. Couldn't a Java/Python/.NET/etc.. SDK have the same issue as well?

@MSNev
Copy link
Collaborator

MSNev commented Nov 15, 2023

The other languages are controlled differently to the JS repo, and I'm not sure if they have any instrumentation key validation or the ability to have the endpoint declared differently. So this would need an issue raised on each repo.

@sander1095 sander1095 changed the title [BUG] Missing disableConnectionStringValidation property which blocks migration [BUG] using EndPointUrl (and IngestionEndpoint) results in Telemetry sent to incorrect urls Nov 17, 2023
@sander1095
Copy link
Author

sander1095 commented Nov 17, 2023

For future readers:

Until this bug is fixed, if you use @microsoft/applicationinsights-web": "^3.0.5 with the connectionString approach and want to change the ingestionEndpoint to anything that isn't the same domain as the application is running on (like https://localhost:3434 for example), you'll run into a bug you can't work around.

If you have set up your endpointUrl to be /hello (for example), the bug is that the url will ALWAYS be /hello/v2/track. The library currently forcibly adds /v2/track to the URL and you can't do anything about it. For example: https://localhost:3434/v2/track.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants