-
Notifications
You must be signed in to change notification settings - Fork 364
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
[SDK-2172] Add SDK metrics to all API calls #659
Conversation
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.
Looks good, just had a minor point around tests.
'Auth0-Client': btoa( | ||
JSON.stringify({ | ||
name: 'auth0-spa-js', | ||
version: version | ||
}) | ||
) |
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.
As we're adding coverage here, it would be worth adding the same assertions on the headers for the test that follows ("..when using refresh tokens"). Looks like an omission testing for one but not the other, even though in reality we know both are covered.
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.
Ye I thought about it, wasn't sure and decided to not pollute all tests with it. But I think I can add it.
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.
Updated the PR to address this. Also added it on a few more places and used DEFAUKT_AUTH0_CLIENT where appropriate
The requests to the /token endpoint did not include the
Auth0-Client
header, making it harder to pull metrics from our data.This PR ensures the token endpoint gets added the appropriate header in the following situations: