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

Use Azure SDK exporter #1653

Merged
merged 52 commits into from May 7, 2021
Merged

Use Azure SDK exporter #1653

merged 52 commits into from May 7, 2021

Conversation

trask
Copy link
Member

@trask trask commented Apr 25, 2021

This is a big change that needs some time to bake, so bumping the version to 3.1.0, and will create 3.0.x branch to continue services releases for 3.0.x until this is ready for prime time.

Many things still to do:

  • Http proxy configured in json is not used yet
  • Data is sent to path /v2//track instead of /v2/track (see Make changes Monitor.Exporters preview swagger file Azure/azure-rest-api-specs#14176)
  • Data is not being sent gzipped
  • Data is not being sent with ndjson
  • Batch sending (using the underlying swagger client) is not implemented yet
  • Truncating telemetry fields is not implemented (e.g. see the @Ignore(d) JDBC smoke test)
  • Store telemetry to disk on failure and retry and throttling (this is significant work, not going to be in this PR)
  • All friendly error messages need to be revisited (many got deleted in telemetry channel)
  • Note there's a breaking configuration change:
  • Check jar file size
  • Performance testing (there were some low-level memory optimization done in previous json bindings, make sure new stuff is at least as good)
  • Reconcile com.microsoft.applicationinsights.agent.Exporter and com.azure.monitor.opentelemetry.exporter.AzureMonitorTraceExporter (may need to wait for OpenTelemetry logs)
  • Lots of smaller misc FIXME (trask) comments left lying around to deal with
  • For attach scenarios, service profiler will be looking for APPLICATIONINSIGHTS_AUTHENTICATION_STRING env variable to enable aad authentication

@lgtm-com
Copy link

lgtm-com bot commented Apr 26, 2021

This pull request introduces 1 alert and fixes 2 when merging 472f7d8 into 75d29bd - view on LGTM.com

new alerts:

  • 1 for Race condition in double-checked locking object initialization

fixed alerts:

  • 1 for Potential output resource leak
  • 1 for Potential input resource leak

@lgtm-com
Copy link

lgtm-com bot commented Apr 26, 2021

This pull request fixes 2 alerts when merging 62be412 into 75d29bd - view on LGTM.com

fixed alerts:

  • 1 for Potential output resource leak
  • 1 for Potential input resource leak

@lgtm-com
Copy link

lgtm-com bot commented Apr 27, 2021

This pull request fixes 2 alerts when merging 63483c6 into 75d29bd - view on LGTM.com

fixed alerts:

  • 1 for Potential output resource leak
  • 1 for Potential input resource leak

@lgtm-com
Copy link

lgtm-com bot commented Apr 27, 2021

This pull request fixes 2 alerts when merging 8b5e3cf into 75d29bd - view on LGTM.com

fixed alerts:

  • 1 for Potential output resource leak
  • 1 for Potential input resource leak

@trask trask changed the base branch from master to aad April 27, 2021 18:32
@trask trask marked this pull request as ready for review April 27, 2021 18:32
@@ -722,14 +706,14 @@ public static void main(String[] args) {
@SuppressWarnings("unused")
public static class TC {

private TelemetryConfiguration configuration;
private com.microsoft.applicationinsights.TelemetryConfiguration configuration;
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious to know why removing imports and using explicit types?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is commented out code, wanted to it compile when uncommenting it, without having to add the import statements after uncommenting it

@@ -85,7 +85,7 @@

public static class Sampling {

public double percentage = 100;
public float percentage = 100;
Copy link
Contributor

Choose a reason for hiding this comment

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

We use double everywhere.. wondering why choose to use float here?

Copy link
Member Author

Choose a reason for hiding this comment

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

the new TelemetryItem.setSampleRate takes a float, so that change rippled to several places

@@ -0,0 +1,16 @@
package com.microsoft.applicationinsights;
Copy link
Contributor

Choose a reason for hiding this comment

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

All the new classes are missing copyrights.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 I'll add in separate PR (#1678 is based on this PR already so don't want to cause it problems)

TelemetryConfigurationFactory.INSTANCE.initialize(configuration, buildXmlConfiguration(config));
configuration.getContextInitializers().add(new SdkVersionContextInitializer());
configuration.getContextInitializers().add(new ResourceAttributesContextInitializer(config.customDimensions));
TelemetryClient telemetryClient = TelemetryClient.initActive(config.customDimensions, buildXmlConfiguration(config));
Copy link
Contributor

Choose a reason for hiding this comment

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

should we track config.customDimensions as a feature in statsbeat?
I wasn't aware of this config before. if nobody uses it, we can consider removing it?

does telemetry processor enable users to add custom dimensions?

@trask trask merged commit 83b96b6 into aad May 7, 2021
@trask trask deleted the trask/azure-sdk-exporter branch May 7, 2021 21:19
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

2 participants