-
Notifications
You must be signed in to change notification settings - Fork 197
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
Added customTelemetry subcapability under copilot #2654
Conversation
This pull request contains changes to the runtime.ts file. If you, as the author of this PR, have made changes to the Runtime interface please review RUNTIME.md to determine if a new runtime version is required. Please reply to this comment stating what changes, if any, were made to the runtime object and whether a new runtime version was required. |
change/@microsoft-teams-js-05b6fda8-1215-45ac-b8c5-338fe7d7f77e.json
Outdated
Show resolved
Hide resolved
…e.json Co-authored-by: Erin <erinha@microsoft.com>
* @beta | ||
*/ | ||
export function sendCustomTelemetryData( | ||
name: Stage = Stage.STAGE_E, |
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.
I'd prefer that this was a UUID instead of an enum for a few reasons:
- If we use an enum, any time a new stage is added the enum will have to be updated
- If they want to gather telemetry for something that is not stage related, we will need to update this
A UUID will allow them to pass in any UUID they want and define what it means for themselves. It also ensures that they don't inadvertently pass personal information that can't be added to telemetry in because we will validate that the UUID is a UUID and not an arbitrary string.
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.
I could be misunderstanding, but Copilot and the host would communicate which UUID means what so they can correlate it and understand what the timestamp represents.
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.
Since the meaning of the name
isn't strictly relevant to the SDK, this allows the app and host to work together to define well-understood values that they can use to correlate any type of timestamp data in a meaningful way, without having to make SDK updates.
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.
@TrevorJoelHarris I have updated the PR with the changes. Please take a look and let me know if that is the correct way.
change/@microsoft-teams-js-05b6fda8-1215-45ac-b8c5-338fe7d7f77e.json
Outdated
Show resolved
Hide resolved
Approved too early. I'm still waiting for a reply to one of my comments.
packages/teams-js/src/index.ts
Outdated
@@ -1,2 +1,3 @@ | |||
export * from './private/index'; | |||
export * from './public/index'; | |||
export { UUID } from './internal/uuidObject'; |
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.
Wouldn't exporting this internal
function defeat the purpose of it being internal
? Should we consider moving this function to public instead?
@@ -0,0 +1,7 @@ | |||
{ | |||
"type": "minor", | |||
"comment": "Added `customTelemetry` capability under `copilot` to send app loading data to the host. The capability is still awaiting support in one or most host applications. To track availability of this capability across different hosts see https://aka.ms/capmatrix", |
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.
"comment": "Added `customTelemetry` capability under `copilot` to send app loading data to the host. The capability is still awaiting support in one or most host applications. To track availability of this capability across different hosts see https://aka.ms/capmatrix", | |
"comment": "Added `customTelemetry` capability under `copilot` to send app loading data to the host", |
Can you say more about why we need this as the default export? Is exporting the class above enough? Default exports are generally a bad idea in JavaScript and using them has caused bugs in the host SDK. Refers to: packages/teams-js/src/internal/uuidObject.ts:33 in 90ca82b. [](commit_id = 90ca82b, deletion_comment = False) |
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.
🕐
Moving the uuidObject.ts file to the public folder in this PR: #2671 So we can use it in this PR correctly. |
Looks like the default export line can be removed and it will still compile fine. |
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.
For more information about how to contribute to this repo, visit this page.
Description
Enable the Copilot app to send custom telemetry data to host apps for tracking UI element loads within Biz Chat.
Main changes in the PR:
Validation
Validation performed:
Unit Tests added:
<Yes/No>
End-to-end tests added:
No
Additional Requirements
Change file added: Yes
Screenshots: