microsoft-authentication-library-for-dotnet
microsoft-authentication-library-for-dotnet copied to clipboard
AzureAD
Add api for telemetry client
Fixes # #3533
Changes proposed in this request Add api for telemetry client as experimental
Testing Unit test
Performance impact
Documentation
- [ ] All relevant documentation is updated.
Thanks Neha, I likely won't get the chance to take a look today but plan to block some time for a review tomorrow.
In reply to: 1209954009
Approving, but would like @TimHannMSFT 's opinion on the public API. We should have the same entry point, i.e. a collection of
ITelemetryClient. Maybe we should use the same type of collection too? Array or IEnumerable or something.
For middleware libraries we don't have any DI concepts so the entry point is new constructors which take in a single ITelemetryClient or an IEnumerable<ITelemetryClient> (either way it's added to the internal set which is initialized to empty by default.)
Yea, I think this work was just split into more manageable pieces. My 2 cents - I would only merge this and other related PRs when the full feature is ready, though.
Looks like this change merely allows an entry point to add an ITelemetryClient to the MSAL instance. I'm assuming a later change will be added which will actually instrument some parts of MSAL to send events to the ITelemetryClient if enabled for the respective client?
I think that's fine to do, perhaps clarifying in the release notes somewhere that this change merely adds the foundation for future capabilities and isn't meant to add any new capabilities on its own.
Yes, correct, this just adds a new public API but no data collection occurs.
rely allows an entry point to add an ITelemetryClient to the MSAL instance. I'm assuming a later change will be added which will actually instrument some parts of MSAL to send events to the ITelemetryClient if enabled for the respective client?
I think that's fine to do, perhaps clarifying in the release notes somewhere that this change merely adds the foundation for future capabilities and isn't meant to add any new capabilities on its own.
The follow up PR is ready for review. I will address these comments and merge it in the other PR. https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/pull/3574
All the comments here are addressed and I have merged this PR into https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/pull/3574. Closing this one.