Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

Comments

fix: telemetry logger middleware#2800

Merged
cwhitten merged 4 commits intomasterfrom
qika/fixconfig
Apr 29, 2020
Merged

fix: telemetry logger middleware#2800
cwhitten merged 4 commits intomasterfrom
qika/fixconfig

Conversation

@zidaneymar
Copy link
Contributor

@zidaneymar zidaneymar commented Apr 28, 2020

Description

  1. Remove UseTelemetryLoggerMiddleware section in appsettings.json, use telemetry section to control
  2. Add feature section to compose default settings page.

Task Item

fixes #2789

Screenshots

@zidaneymar
Copy link
Contributor Author

@luhan2017 Hi Lu, could you please review this pr about the settings part? Do we need to keep the UseTelemetryLoggerSettings or just use the 'telemetry' section to maintain.

@github-actions
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling 98a9e07 on qika/fixconfig into ac1e959 on master.

UseTranscriptLoggerMiddleware: false,
UseShowTypingMiddleware: false,
UseInspectionMiddleware: false,
UseCosmosDb: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this setting name is ambiguous and confusing. I had to check if this meant use CosmosDb instead of another persistent storage (e.g. Blob storage) or if it was simply enabling persistent storage in favor of using memory, where CosmosDb is the only option.

In my opinion, we need to be more explicit here and change this to either UsePersistentStorage or UseCosmosDbPersistentStorage.

Copy link
Contributor

@luhan2017 luhan2017 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to move all those settings to the bot\settings\appsettings.json page, and so that user can edit it in composer, otherwise user can't see this. in appsettings.json, we can only keep bot and root to specify the bot location.

@zidaneymar
Copy link
Contributor Author

I think we need to move all those settings to the bot\settings\appsettings.json page, and so that user can edit it in composer, otherwise user can't see this. in appsettings.json, we can only keep bot and root to specify the bot location.

yes actually I add a default settings in Composer to include the runtime settings, I'm not sure, is this config section reasonable?

protected createDefaultSettings = (): any => {
    return {
      feature: {
        UseTranscriptLoggerMiddleware: false,
        UseShowTypingMiddleware: false,
        UseInspectionMiddleware: false,
        UseCosmosDb: false,
      },
      MicrosoftAppPassword: '',
      MicrosoftAppId: '',
      luis: {

@zidaneymar zidaneymar marked this pull request as ready for review April 29, 2020 06:07
@garypretty garypretty self-requested a review April 29, 2020 15:55
Copy link
Contributor

@beyackle beyackle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing in here jumps out at me as wrong, and it already got the ✅ from someone who had requested changes, so I'll carry that forward.

@cwhitten cwhitten merged commit ea9af87 into master Apr 29, 2020
@cwhitten cwhitten deleted the qika/fixconfig branch April 29, 2020 23:15
lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
* fix telemetry client.

* change telemetry default to true, change naming

Co-authored-by: Ben Yackley <61990921+beyackle@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants