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

Added application insights starter for spring boot #518

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
* @author Arthur Gavlyukovskiy
*/
@Configuration
@ConditionalOnBean(TelemetryConfiguration.class)
public class ApplicationInsightsModuleConfiguration {

@Bean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@
* @author Arthur Gavlyukovskiy
*/
@Configuration
@ConditionalOnBean(TelemetryConfiguration.class)
@ConditionalOnProperty(value = "azure.application-insights.enabled", havingValue = "true", matchIfMissing = true)
@ConditionalOnWebApplication
public class ApplicationInsightsWebModuleConfiguration {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ public FilterRegistrationBean webRequestTrackingFilterRegistrationBean(WebReques

@Bean
@ConditionalOnMissingBean
public WebRequestTrackingFilter webRequestTrackingFilter(@Value("${spring.application.name:application}") String applicationName) {
public WebRequestTrackingFilter webRequestTrackingFilter(@Value("${spring.application.name:application}") String applicationName,
// we have implicit dependency on configured telemetryConfiguration here
@SuppressWarnings("unused") TelemetryConfiguration telemetryConfiguration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@gavlyukovskiy Can you explain the need of this? I do understand in the changes above you made this conditional on web application which is auto configured after the default configuration. Is this what you are referring to and believe that TelemetryConfiguration would always be initialized ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AutoConfigureAfter and @AutoConfigureBefore sets the order of creating bean definitions, but the beans itself are created in different order which controlled by dependency graph. I set explicit dependency on TelemetryConfiguration to force it to be created before WebRequestTrackingFilter to ensure that call TelemetryConfiguration#getTelemetryModules will return all modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gavlyukovskiy thank you very much. Also about the sampling do you have any idea on getting that in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I'm following the full flow how channel and and processor work together. I understood processor controls whether telemetry should be sent and channel has settings to control telemetry sending, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes so if processor returns false then the telemetry is dropped and doesn't hit transmission channel. So processor controls if telemetry should be sent or not. Channel is responsible for ultimately pumping the telemetry and it has several characterstics which tell what is the end point what is the buffer size etc.

Copy link
Contributor Author

@gavlyukovskiy gavlyukovskiy Feb 25, 2018

Choose a reason for hiding this comment

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

And for what TelemetrySampler is used? Should we drop FixedRateTelemetrySampler?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please drop it. It is not a recommended way to sample. We should mark it as deprecated also :-)

Copy link
Contributor Author

@gavlyukovskiy gavlyukovskiy Feb 25, 2018

Choose a reason for hiding this comment

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

What do you think about moving these types of telemetry to enum:

  • Dependency
  • Event
  • Exception
  • PageView
  • Request
  • Trace

? With that we can make autocomplete in properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gavlyukovskiy I am okay with this unless this is something globally exposed. Autocompletion would be desirable but I would like to understand what could be the implications if this is globally exposed because we won't be able to remove this until next major version and making it public then people can take explicit dependency on this enum and if we want to make it public in any situation then it should not reside in this processor but rather should have it's home somewhere else where it is more easy to locate and used in other parts of the code base.

return new WebRequestTrackingFilter(applicationName);
}
}
Expand Down