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

TelemetryLoggerMiddleware #1420

Merged
merged 4 commits into from
Mar 17, 2019
Merged

TelemetryLoggerMiddleware #1420

merged 4 commits into from
Mar 17, 2019

Conversation

daveta
Copy link
Contributor

@daveta daveta commented Mar 6, 2019

Middlware component that uses IBotTelemetryClient to log events when messages are received/sent/updated or deleted in the bot.

Additional Details
https://github.com/daveta/analytics/blob/master/telemetry_enhancements/TelemetryEnhancements.md

js: microsoft/botbuilder-js#812

{
public static class TelemetryConstants
{
public const string ChannelIdProperty = "channelId";
Copy link
Contributor

Choose a reason for hiding this comment

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

Should all telemetry properties follow the same casing? This middleware & the luis/qna samples use lower camel case. The waterfall telemetry & initializer is upper camel case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh.
The Initializer properties begin with lowercase, so we're already inconsistent. I'm leaning towards aligning with Initializer and begin with lowercase character - any thoughts?

Tempted to lowercase waterfall telemetry, but that's actually a breaking change.


In reply to: 263166401 [](ancestors = 263166401)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with lowercase

@coveralls
Copy link
Collaborator

coveralls commented Mar 7, 2019

Pull Request Test Coverage Report for Build 51486

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 45 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.8%) to 75.316%

Files with Coverage Reduction New Missed Lines %
/libraries/integration/Microsoft.Bot.Builder.Integration.AspNet.Core/BotFrameworkHttpAdapter.cs 3 66.67%
/libraries/Microsoft.Bot.Builder.AI.LUIS/LuisRecognizer.cs 42 71.52%
Totals Coverage Status
Change from base Build 50252: 0.8%
Covered Lines: 4226
Relevant Lines: 5611

💛 - Coveralls

@daveta daveta changed the title [DO NOT MERGE] TelemetryLoggerMiddleware TelemetryLoggerMiddleware Mar 10, 2019
@daveta daveta merged commit 153df10 into master Mar 17, 2019
@cleemullins cleemullins deleted the daveta-telemetry branch September 12, 2019 22:47
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.

None yet

4 participants