Add telemetry only for requests to graph.microsoft.com #336
Conversation
| * @param {string} url - The request url string | ||
| * @returns true if request url is a Graph url | ||
| */ | ||
| private isGraphURL(url: string): boolean { |
There was a problem hiding this comment.
- construct a
Set()object forgraphURLswhere the values are only the host names. - construct a typed
URLobject forurl. - Have this function return the result of
graphURLs.has(urlObject.hostname)
Benefits:
- Efficiency: Lookup is amortized O(1).
- Correctness: We probably want to capture telemetry for urls like:
https://graph.microsoft.com:443and current implementation will not accept URLs with port numbers.
There was a problem hiding this comment.
also consider inlining the whole function as it will be a simple call to graphURLs.has(new URL(context.request).hostname)
There was a problem hiding this comment.
Thank you for suggesting an optimal approach!
I have used the approach you suggested with some modifications. Implemented a set object with the host names. I haven't used the URL object as some browsers do not support of the URL object and I am extracting the host name as it is done in GraphRequest.ts during the url parsing process.
|
|
||
| // using an IIFE to populate the set object with the graph host names as Set(iterable:Object) is not supported in Internet Explorer | ||
| (() => { | ||
| const urls = ["graph.microsoft.com", "graph.microsoft.us", "dod-graph.microsoft.us", "graph.microsoft.de", "microsoftgraph.chinacloudapi.cn"]; |
There was a problem hiding this comment.
you could initialize the set directly from the array, see the edit in the response.
https://stackoverflow.com/a/30902786/3808675
There was a problem hiding this comment.
I used an IIFE with set.add() instead as IE does not support Set(array). IE creates an empty set when I passed in new Set(object/array).
There was a problem hiding this comment.
interesting, and I'm guessing this is an edge case the transpilation is not catching?
There was a problem hiding this comment.
additionally this is something that people (or we) can polyfill using core-js, see the first example. https://www.npmjs.com/package/core-js
| const endOfHostStrPos = url.indexOf("/"); | ||
| if (endOfHostStrPos !== -1) { | ||
| // Parse out the host | ||
| const hostName = url.substring(0, endOfHostStrPos); |
There was a problem hiding this comment.
this doesn't cover the scenario of https://graph.microsoft.com:443/v1.0/me
|
Will create another pull request dealing with custom headers. |
3 participants
Fixes #265