Conversation
|
@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. |
Composer/packages/server/src/models/settings/defaultSettingManager.ts
Outdated
Show resolved
Hide resolved
| UseTranscriptLoggerMiddleware: false, | ||
| UseShowTypingMiddleware: false, | ||
| UseInspectionMiddleware: false, | ||
| UseCosmosDb: false, |
There was a problem hiding this comment.
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.
luhan2017
left a comment
There was a problem hiding this comment.
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? |
beyackle
left a comment
There was a problem hiding this comment.
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.
* fix telemetry client. * change telemetry default to true, change naming Co-authored-by: Ben Yackley <61990921+beyackle@users.noreply.github.com>
Description
Task Item
fixes #2789
Screenshots