SkillConversationIdFactory in Skills samples does not create unique key if Root Bot consumes multiple Skills

[Zerryth]

Sample information

1. Sample type: samples
2. Sample language: across all SDKs
3. Sample name: 80.skills-simple-bot-to-bot

Describe the bug

The team seems to be sending the message that a Skill Consumer can consume multiple Skills And in order to implement skills, you must create a SkillConversationIdFactory

However the SkillConversationIdFactory that we show in the sample as we have it would not allow the scenario to have one root bot consume multiple skills, as the way we create the key for skill conversation ids would create the same key for multiple skills within the same conversation:

In SkillConversationIdFactory:

var key = $"{conversationReference.Conversation.Id}-{conversationReference.ChannelId}-skillconvo";

Expected behavior

To have unique skill conversation id keys (to ultimately have a skill consumer be able to consume multiple skills)

Would suggest appending some unique identifier on the skill into the key, like the skill's appId or id

[bug]

[gabog]

Not really sure I understand what the but is. Our current framework only allows to interact with one skill at a time. It should be ok use the same conversation ID for two different skills. Have you run into any issues doing this?

We will change this in R8 to address other scenarios related to adaptive and oauth but I'm curious to know if you had any problems sharing the same conversation id with multiple skills.

[Virtual-Josh]

@Zerryth Can you please elaborate for Gabo? If this isn't an issue can you close?

[Zerryth]

Sorry Gabo! Didn't realize you commented!

Hmm...perhaps I am mistaken?

I was thinking the scenario that if I had a Root bot that called 2 skills (say Checkout skill and CustomerService skill), would I not want to have 2 different keys then for Root-Checkout and Root-CustomerService to ensure that they are in 2 separate conversations?

If the key is created with ChannelId:Convo.Id, then wouldn't there be a risk of having Root-Checkout and Root-CustomerService have the same conversation reference, then?

[Zerryth]

But I see, if we're only interacting with 1 skill/time, then there wouldn't be a need to have 2 root-skill level conversation references

Made this issue when I was making a higher-level UML diagram (for reference):

• A root bot can consume many skills
• A skill can be consumed by many bots

[EricDahlvang]

I can envision a group conversation where multiple users, and multiple bots, are part of the conversation. In this case: the channel's conversation.id will be the same for all skills, and users in the conversation. Our current implementation does not support this scenario. However, adding BotFrameworkSkill as a parameter to CreateSkillConversationIdAsyncwould enable skill specific conversation ids.

[stevengum]

tl;dr Skip to Alternate Solution for m*n SkillConversationIds

1 Parent, n skills & 1 SkillConversationId per ConversationReference

In the samples, n skills will receive the same SkillConversationId and any skill-sent message will use the same ConversationReference when being forwarded.

To each skill, we can say this conversationId from the parent is unique. The skills aren't necessarily aware of the other skills, so they're not worrying about the uniqueness of their conversationId. I'm not sure they ever would.

For any activities using this SkillConversationId, the parent knows who sent it (via the ClaimsIdentity) and has the Factory to get the correct ConversationReference.

---

1 Parent, n skills & n SkillConversationIds per ConversationReference

The developer decides to make a SkillConversationIdFactory implementation that guarantees unique SkillConversationIds for each skill. It does it by using the SkillAppId.

Now we have 1 Conversation Reference to n SkillConversationIds.

These unique keys point to the same ConversationReference that's been squirreled away, so it's just a duplication of data. The bot knows when it send an activity to a skill, because the information is provided when it calls SkillHttpClient.PostActivityAsync().

All messages from the parent look like they're coming from the parent, and they all go to the same conversation.

m Parents, n skills and using the SkillAppId to create the SkillConversationId?

In the diagram above, we're assuming that each parent bot in the conversation does the following:

• They share skills
• They implement their SkillConversationId-generating algorithm the same way

Not all future real world applications will duplicate this specific scenario.

With the proposed changes, passing in a BotFrameworkSkill instance to CreateSkillConversationIdAsync() doesn't guarantee that the Factory implementation uses the SkillAppId for each SkillConversationId. It's an abstract class, not an implementation. We're in the sample repo, but we're discussing SDK-level changes.

If each parent is in the same conversation as the next, and all use the Skill's AppId in calculating the SkillConversationId in the same manner, we ultimately end up with 1 SkillConversationId being received from m parents. Which is a worse version first scenario with a slightly longer key.

At best we've enabled one solution for the diagram, worse it's a parameter that no one uses, worse yet, it hasn't solved the original problem.

---

Alternate solution

(m parents, n shared skills, m*n SkillConversationIds)

If the concern is for the skills receiving the same SkillConversationId from multiple parents, then have the parents use their own AppId when generating a SkillConversationId:

// _botId is the parent's AppId.
var key = $"{_botId}:{conversationReference.Conversation.Id}-{conversationReference.ChannelId}-skillconvo";

This ensures the SkillConversationId is different for each parent bot. So the skill has m conversationIds that ultimately resolve to the same ConversationReference (at m parents).

Passing in the BotFrameworkSkill to use the Skill's AppId could have resulted in the same problem as outlined by the diagram. Using the parent's AppId results in the following:

m*n SkillConversationIds, m instances of the 1 ConversationReference across m storage providers

Nit: m parents actually means m ConversationReferences because the Bot in each Reference is different...

public ConversationReference GetConversationReference()
{
    ConversationReference reference = new ConversationReference
    {
        ActivityId = this.Id,
        User = this.From,
        Bot = this.Recipient,
        Conversation = this.Conversation,
        ChannelId = this.ChannelId,
        ServiceUrl = this.ServiceUrl,
    };

    return reference;
}

https://github.com/microsoft/botbuilder-dotnet/blob/5f443353ac3b6c5c8c23fb1a4545fa9c61ed7c74/libraries/Microsoft.Bot.Schema/ActivityEx.cs#L443-L456

@Zerryth & @EricDahlvang does this address the concern(s) raised in the issue?

[EricDahlvang]

I don't think this fully addresses the issue Gabo raised yesterday, where the parent might wish to have a side-conversation based on a specific action, then continue the previous conversation when the side-conversation ends.

[Zerryth]

+1 for @EricDahlvang 's comment. As Gabo mentioned yesterday, the interruption scenario is not addressed

And it's not an unlikely real-world scenario.

Say someone was using a personal assistant bot which is all the rage nowadays, and maybe it's in a Movies skill (playing movies, movies catalog, etc.)--then they remember something, and want to take a note. Personal assistant bot might have a Note skill completely separate from a Movie skill, and would need to be able to jump from Movie, to Note, then back to Movie again.

Or I think the example that Gabo gave was Booking a Flight, but what if you wanted to call a Weather skill before you decide on the flight's destination, you'd need to be able to switch back and forth, or the user would need to restart the booking again.

---

Uy my brain hurts, does that mean we need Consumer AppId + SkillId for the key? So that:

• Skills can have conversations with the right parent
• Parent can resume conversation into the right skill, having a side-convo

[EricDahlvang]

A key for all scenarios might be something like:

var key = $"{_botId}-{skillId}-{conversationReference.Conversation.Id}-{conversationReference.ChannelId}-{actionTypeOrId}";

[gabog]

This will be partially addressed in #2245, where we are using the new SkillConversationIdFactoryBase methods. The new CreatedSkillConversationIdAsync method takes now a SkillConversationIdFactoryOptions which contains an instance of the SkillInfo and can be used to build a key with the skill App ID. It does not include the action, if a dev needs to have different conversation ID for different actions in the same skill, it will need to extend SkillConversationIdFactoryOptions to contain an action ID and update the factory to use this property.