-
Notifications
You must be signed in to change notification settings - Fork 88
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
Track time it takes to get a response from DevX API #875
Conversation
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-875.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-875.centralus.azurestaticapps.net |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion
Instead of using trackApiCallEvent
to make network requests consider overriding fetch
because developers won't have to remember to use trackApiCallEvent
instead of fetch
and this approach also maintains the intuitiveness of using fetch
for network calls.
You can override fetch with the following snippet.
windows.fetch = telemetry.trackedApiCall
Remember to do this in App.tsx and to change the signature of trackApiCallEvent
so that the componentName
param is last that way you'll avoid breaking calls to fetch that you are not interested in tracking.
@jobala I wouldn't recommend changing a global function at the JavaScript API level. @millicentachieng network calls to DevX API are failing when I use the static deployment. Constructed URL seems to be wrong. Is that expected?
|
…ph-explorer-v4 into feature/trackDevXAPICalls
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-875.centralus.azurestaticapps.net |
Whether it's opt in depends on comfort level that we are properly sanitized. I think there's benefit in telemetry automatically being available provided we trust it's sanitized and it doesn't generate massive increases in data costs. The typical pattern I've seen in the past is that we actually put this as part of middleware in the http pipeline which generally automatically covers everyone. I don't have a problem with this current approach but when someone wants to add a new call the answer will probably be yes we should track it so the pure fetch call should almost never be used. My expectation would have been that trackApiCallEvent accepts the function so it's explicit what's been called. That way you can just expose from telelmetry class a trackevent and it becomes non-specific to making a web call or some other action we want to track. I see though that you want information from the parameters and response so that limits that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion 2: Middleware
We can take advantage of redux middleware which will allow us to keep the code as-is and add the side-effects in a more react way.
Middleware has access to the state and action properties which would be useful to create the custom event. It would also give us the advantage of one source of truth for when we need to change/add/remove a parameter to track.
Here is a short introduction to redux middleware.
It makes sense for users to explicitly opt in to track requests and we can make the design better by separating the tracking and network request functionality because it is not obivous from the name that The tracking and network call can be separated using the decorator pattern and we'll have the added advantage of not modifying @telemetry.trackApiCallEvent
fetch(...) The decorator implementation below shows how you can use a decorator to get the elapsed time. const trackApiCallEvent = (
target: Object,
propertyKey: string,
descriptor: PropertyDescriptor
) => {
const method = descriptor.value;
descriptor.value = function (...args) {
const start = ...
const result = method.apply(this, args);
const finish = ...
const elapsedTime = finish - start
return result;
}; EDIT:
|
@jobala, @thewahome I've reviewed your comments on using a decorator or middleware but I have another suggestion. Application Insights has a setting where you can enable auto-tracking of fetch calls. The result is what we are looking for. Here's a sample of the data collected:
We can then have a processor that filter's out fetch requests that we do not need. |
This looks good!! Would the preprocessor be able to also filter out the url contents for PII? |
Do we plan on using it for all our external calls? Including the run query? |
@thewahome Yes, the processor will sanitize the urls. |
It also captures results from run query. I'm thinking that can be an important metric too to see which queries have a high failure rate, whether caused by missing permissions or wrongly formatted query. |
71b1323
to
daeb51b
Compare
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-875.centralus.azurestaticapps.net |
1 similar comment
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-875.centralus.azurestaticapps.net |
@thewahome, @jobala Please review the new changes |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-875.centralus.azurestaticapps.net |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we performed a review of the Application Insights data captured with these changes to verify that we aren't capturing customer data? What were the user actions in the staging site and did they exercise all of the changes in the this PR?
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-875.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-875.centralus.azurestaticapps.net |
@millicentachieng If you'd like to test the telemetry on the staging site, you can:
Once the staging environment is created, you will be able to test your changes |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-875.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-875.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-875.centralus.azurestaticapps.net |
Overview
Add a custom event which allows us to capture the time it takes for a call to our DevX API to return a response.
Application Insights JS SDK offers two methods to use to track events:
appInsights.startTrackEvent(eventName)
, to be called just before the tracked callappInsights.stopTrackEvent(eventName, properties)
to be called just after the tracked callApplication Insights will add a custom property
duration
with the API call duration in milliseconds.Testing Instructions