Azure / botbuilder-instrumentation

Add extra logging for bot framework
MIT License
23 stars 22 forks source link

what's the reasoning for only setting the "autoCollect" params for just the 1st AppInsightsKey? #17

Open user1m opened 7 years ago

user1m commented 7 years ago

@morsh In the code below what's the reasoning for only setting the "autoCollect" params for just the 1st AppInsightsKey?

  private setupInstrumentation() {
    if (this.instrumentationKeys && this.instrumentationKeys.length > 0) {
      //we are setting the automatic updates to the first instumentation key.
      let autoCollectOptions = this.settings && this.settings.autoLogOptions || {};
      ApplicationInsights.setup(this.instrumentationKeys[0])
        .setAutoCollectConsole(autoCollectOptions.autoCollectConsole || false)
        .setAutoCollectExceptions(autoCollectOptions.autoCollectExceptions || false)
        .setAutoCollectRequests(autoCollectOptions.autoCollectRequests || false)
        .setAutoCollectPerformance(autoCollectOptions.autoCollectPerf || false)
        .start();

      //for all other custom events, traces etc, we are initiazling application insight clients accordignly.
      let self = this;
      _.forEach(this.instrumentationKeys, (iKey) => {
        let client = ApplicationInsights.getClient(iKey);
        self.appInsightsClients.push(client);
      });
    }
  }
morsh commented 7 years ago

Hi @User1m - I think this was a quick fix since the code was supposed to support multiple application insights accounts, but wasn't really able to support them (although I might be mistaken). Also, I think that ApplicationInsights.setup can only run once.

user1m commented 7 years ago

Ahh ok if ApplicationInsights.setup can only run once is correct then I see.

morsh commented 7 years ago

@User1m - I think so, I havn't had a chance to test multiple AI accounts yet