Skip to content
This repository was archived by the owner on Jun 10, 2020. It is now read-only.

Make QuickPulse configurable by adding it to DI like all other modules#645

Merged
cijothomas merged 6 commits intodevelopfrom
cithomas/fixqpconfig
Apr 12, 2018
Merged

Make QuickPulse configurable by adding it to DI like all other modules#645
cijothomas merged 6 commits intodevelopfrom
cithomas/fixqpconfig

Conversation

@cijothomas
Copy link
Copy Markdown
Contributor

@cijothomas cijothomas commented Apr 12, 2018

Add QuickPulseTelememodule to DI so as to enable retrieval and configuration of the same by user.

QP is enabled by configuring it based on the user supplied flag EnableQuickPulse as before. But now users can use the extension method services.ConfigureTelemetryModule() to configure QP module.

Fix Issue #639

  • I ran Unit Tests locally.

For significant contributions please make sure you have completed the following items:

  • Changes in public surface reviewed

  • Design discussion issue #

  • CHANGELOG.md updated with one line description of the fix, and a link to the original issue.

  • The PR will trigger build, unit tests, and functional tests automatically. If your PR was submitted from fork - mention one of committers to initiate the build for you.
    If you want to to re-run the build/tests, the easiest way is to simply Close and Re-Open this same PR. (Just click 'close pull request' followed by 'open pull request' buttons at the bottom of the PR)

  • Please follow [these] (https://github.com/Microsoft/ApplicationInsights-aspnetcore/blob/develop/Readme.md) instructions to build and test locally.

…uration of the same by user. QP is enabled by configuring it based on the user supplied flag EnableQuickPulse as before.
@cijothomas cijothomas requested a review from netgh0st April 12, 2018 17:41
@cijothomas
Copy link
Copy Markdown
Contributor Author

cijothomas commented Apr 12, 2018

QP module will be present in DI always even when QP is explicitly disabled by user. However, if disabled, the QP TelemetryProcessor won't be created and added to QPModule. - Net effect is that, if QP is disabled by user, then it will be disabled, though there is a QP module sitting idle in DI.

This may/may not be ideal - but this fixes the important issue of 'Not able to configure QP' for immediate term.

The full config story will be revisited soon.

@cijothomas
Copy link
Copy Markdown
Contributor Author

I'll send separate PR's to Doc update this doc which is incorrect.
https://docs.microsoft.com/en-us/azure/application-insights/app-insights-live-stream#secure-the-control-channel

}

[Event(12, Message = "Unable to find expected module {0} in service collection.", Level = EventLevel.Warning, Keywords = Keywords.Diagnostics)]
public void UnableToFindExpectedModule(string moduleType, string appDomainName = "Incorrect")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you make this message more specific. So customer will know that without this QuickPulse wouldn't work. Generic message doesn't help.

Also promote it to Error so support of customer screwed up config and not able to see live metrics would be easier

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Contributor

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

Please change the text to make it easier to support the product. So reading an error person can figure out what's wrong

this.WriteEvent(11, moduleType, this.ApplicationName);
}

[Event(12, Message = "Unable to find QuickPulseTelemetryModule in service collection. QuickPulse/LiveMetrics feature will not be available.", Level = EventLevel.Error, Keywords = Keywords.Diagnostics)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

pick a name =) Either quick pulse or Live Metrics. Also suggest a fix, Make it easy to support the product

@cijothomas cijothomas merged commit 7fd14bd into develop Apr 12, 2018
@cijothomas cijothomas deleted the cithomas/fixqpconfig branch April 12, 2018 22:17
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.

2 participants