OfficeDev / TeamsFx

Developer tools for building Teams apps
Other
427 stars 165 forks source link

bot.notification.findMember taking more time to find user. #9734

Closed sid-neo closed 2 months ago

sid-neo commented 8 months ago

Describe the bug bot.notificaiton.findMember is taking minimum more than 2.5 seconds to find an user, and sometime it takes around 4 to 8 seconds We are using below method to find user conversation reference const selectedUser = await bot.notification.findMember(async (m) => m.account.aadObjectId === req.body.user_id);

To Reproduce

Expected behavior We are expecting it to be the notification to find the user a bit faster than this, because overall it takes around more than 4 seconds to final user getting message.

Screenshots

image

image

image

Additional context

Reference code

if (req.body) {
      if (req.body.user_id && req.body.message && req.body.body_type) {
        console.log(new Date().toISOString() + '2. Searching for user');
        const selectedUser = await bot.notification.findMember(async (m) => m.account.aadObjectId === req.body.user_id);
        console.log(new Date().toISOString() + '3. Retrieved User');
        if (selectedUser) {
          if (req.body.body_type === "text") {
            console.log(new Date().toISOString() + '4. Sending Message');
            //removing await for faster execution
            selectedUser.sendMessage(req.body.message);
            console.log(new Date().toISOString() + '5. Message Sent');
            res.json(200, { message: "Message Sent" });
          } else if (req.body.body_type === "adaptivecard") {
            //removing await for faster execution
            selectedUser.sendAdaptiveCard(AdaptiveCards.declare(req.body.message).render());
            res.json(200, { message: "Message Sent" });
          }
        } else {
          res.json(500, { message: "User Not found" });
        }
      } 
microsoft-github-policy-service[bot] commented 8 months ago

Thank you for contacting us! Any issue or feedback from you is quite important to us. We will do our best to fully respond to your issue as soon as possible. Sometimes additional investigations may be needed, we will usually get back to you within 2 days by adding comments to this issue. Please stay tuned.

qinezh commented 8 months ago

@sid-neo Could you share about how many bot installations (personal chat, group chat, channel chat) are there, and how many members are under each of these installations for your bot app?

Underlying, findMember will list all the members belong to these installations. If there're too many members of your installations, it would take quite a while to call Teams API to list all the members before finding the targeted member.

You may consider to set the search scope to find members from the installations (personal chat, group chat, channel chat) to narrow down the search scope to speed up the process:

findMember(predicate: (member: Member) => Promise, scope?: SearchScope)

sid-neo commented 8 months ago

hi @qinezh - thanks for prompt response.... as of now we are in testing phase and we only have 3 users who has installed bot. does this depends on number of users installed ?

sid-neo commented 8 months ago

We will try search scope thing to see if it is making any difference.

qinezh commented 8 months ago

Could you also try below snippet to check the duration to call findAllMembers and the count of members?

const members = await bot.notification.findAllMembers(() => Promise.resolve(true));
console.log(members.length);
kunj-sangani commented 8 months ago

Hi @qinezh

Updated the code as mentioned above now the code looks like

const selectedUser = await bot.notification.findMember(async (m) => m.account.aadObjectId === req.body.user_id, 1);

Outcome after that

image

It took around 11seconds to just fetch the user

qinezh commented 8 months ago

It's weird that there're only 3 users from all your bot installations, I'll take a further look.

torresedgar87 commented 8 months ago

Hello, i am having a similar issue const member = await bot.notification.findMember(m => Promise.resolve(m.account.id === msTeamsId), 1); this is taking a long time. The issue gets worse the more users that install the app.

export const bot = new ConversationBot({
  // The bot id and password to create CloudAdapter.
  // See https://aka.ms/about-bot-adapter to learn more about adapters.
  adapterConfig: {
    MicrosoftAppId: config.botId,
    MicrosoftAppPassword: config.botPassword,
    MicrosoftAppType: 'MultiTenant',
  },
  // Enable notification
  notification: {
    enabled: true,
    storage: new DynamoDBStorage(config.aws.region, config.dynamoDb.tableName),
  },
});

I configured my bot to use DynamoDB for storage and I can tell that on findMember it does a request for all installations. If I look at the data that is stored on every installation conversation info is not included. This means that at some point its also reaching out to some Microsoft external service to get the conversation info before returning on the findMember method. Could this also be considered as a possibility for such a long delay?

qinezh commented 8 months ago

@torresedgar87 Yeah, that's one of the main reasons. When findMember is invoked, it will:

  1. List all installations from the conversation reference store.
  2. Call Teams API to check if above installations are valid and remove the invalid ones.
  3. Call Teams API to get all members from each installation.
  4. Filter and return the particular member from all members.

For step 2 & 3, it might take a long delay to call Teams API and get the response. Especially when there's lots of installations and the number of members is huge.

One approach is to implement your customized conversation reference store using the latest SDK. (see ConversationReferenceStore and here's a sample implementation with Azure Blob).

Then you could add a method to find installations from the customized store with your own logic and convert the found conversation reference to notification target using buildTeamsBotInstallation.

qinezh commented 8 months ago

The default conversation reference store only supports for-loop lookup because it saves conversation reference in a fixed schema in local disk for general purpose, while the customized store can set database index on any field of the conversation reference according to the business requirement.

So, it would be recommended to use a customized conversation reference store to retrieve a conversation reference if there’s performance concern.

gupta-lakshay commented 2 months ago

@qinezh can you please share how are you storing the conversationReference and retrieving the user out of blobstore based on member's email or UPN or aadObjectId

qinezh commented 2 months ago

For the way to store conversation reference, you may refer to the sample implementation with Azure Blob. As for the way to retrieve the user using email or AAD object id, you could add a method like getByUserEmail(email) or getByUserId(userId) in your customized store and call it when needed.

gupta-lakshay commented 2 months ago

For the way to store conversation reference, you may refer to the sample implementation with Azure Blob. As for the way to retrieve the user using email or AAD object id, you could add a method like getByUserEmail(email) or getByUserId(userId) in your customized store and call it when needed.

we have an existing method findMember for ConversationReferenceStore, is it good to use it ? Does it internally uses graph APIs to fetch UPN and do UPN based filtering as UPN is not stored as part of conversation Reference ? Is it a recommendation to implement method getByUserEmail(email) or getByUserId(userId) or use existing findMember method ?

qinezh commented 2 months ago

Yes, the method findMember is good to use and internally it uses BotBuilder SDK to call Teams API to fetch all the members. It would be recommended to use a customized conversation reference store to retrieve a conversation reference if there’s performance concern.

tomflenner commented 1 day ago

Hi 👋🏻 !

Any update on this ? We're facing the same issue.

For the way to store conversation reference, you may refer to the sample implementation with Azure Blob. As for the way to retrieve the user using email or AAD object id, you could add a method like getByUserEmail(email) or getByUserId(userId) in your customized store and call it when needed.

@qinezh Can you help to provide the method getByUserEmail(email) ? 🙏🏻

We use blob storage and our installations are like this :

{
  "activityId": "",
  "user": {
    "id": "",
    "aadObjectId": ""
  },
  "bot": {
    "id": "",
    "name": "teams-bot"
  },
  "conversation": {
    "conversationType": "personal",
    "tenantId": "",
    "id": ""
  },
  "channelId": "msteams",
  "locale": "fr-FR",
  "serviceUrl": "https://smba.trafficmanager.net/emea/"
}

How can we easily search for mail inside this blob ?

Thanks in advance 🙏🏻