Skip to content
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

fix: telemetry logger middleware #2800

Merged
merged 4 commits into from
Apr 29, 2020
Merged

fix: telemetry logger middleware #2800

merged 4 commits into from
Apr 29, 2020

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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants