-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[AzureMonitorAutoConfigure] Align customer SDKStats with stable spec #50056
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
base: main
Are you sure you want to change the base?
Changes from all commits
3c9acd7
00ff7b3
424c528
0aedfb9
a7557fb
bcf04e6
4e56ad5
00a7c78
4b27bf3
417f595
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,7 @@ public void customerSdkStatsEnabledByDefault() { | |
| @Test | ||
| public void customerSdkStatsDisabledByPublicProperty() { | ||
| ConfigProperties config = DefaultConfigProperties | ||
| .createFromMap(Collections.singletonMap("APPLICATIONINSIGHTS_SDKSTATS_DISABLED", "true")); | ||
| .createFromMap(Collections.singletonMap("applicationinsights.sdkstats.disabled", "true")); | ||
| assertThat(AzureMonitorExporterBuilder.isCustomerSdkStatsEnabled(config)).isFalse(); | ||
| } | ||
|
|
||
|
|
@@ -38,16 +38,24 @@ public void customerSdkStatsDisabledByAllProperty() { | |
| @Test | ||
| public void customerSdkStatsDisabledAllTakesPrecedence() { | ||
| Map<String, String> props = new HashMap<>(); | ||
| props.put("APPLICATIONINSIGHTS_SDKSTATS_DISABLED", "false"); | ||
| props.put("applicationinsights.sdkstats.disabled", "false"); | ||
| props.put("APPLICATIONINSIGHTS_SDKStats_DISABLED_ALL", "true"); | ||
| ConfigProperties config = DefaultConfigProperties.createFromMap(props); | ||
| assertThat(AzureMonitorExporterBuilder.isCustomerSdkStatsEnabled(config)).isFalse(); | ||
| } | ||
|
|
||
| @Test | ||
| public void customerSdkStatsDisabledAllFalseLeavesEnabled() { | ||
| Map<String, String> props = new HashMap<>(); | ||
| props.put("APPLICATIONINSIGHTS_SDKStats_DISABLED_ALL", "false"); | ||
| ConfigProperties config = DefaultConfigProperties.createFromMap(props); | ||
| assertThat(AzureMonitorExporterBuilder.isCustomerSdkStatsEnabled(config)).isTrue(); | ||
| } | ||
|
|
||
| @Test | ||
| public void customerSdkStatsDisabledFalseLeavesEnabled() { | ||
| ConfigProperties config = DefaultConfigProperties | ||
| .createFromMap(Collections.singletonMap("APPLICATIONINSIGHTS_SDKStats_DISABLED_ALL", "false")); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question about this deletion as well - I'm reading in the spec that this would have disabled all (customer and internal) sdkstats. Was this deleted because the other var would do the same thing for customer sdkstats? what if someone only set the disabled all - do you see that as a viable usecase? |
||
| .createFromMap(Collections.singletonMap("applicationinsights.sdkstats.disabled", "false")); | ||
| assertThat(AzureMonitorExporterBuilder.isCustomerSdkStatsEnabled(config)).isTrue(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| package com.azure.monitor.opentelemetry.autoconfigure.implementation.utils; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| public class VersionGeneratorTest { | ||
|
|
||
| @Test | ||
| public void sdkVersionNumberIsAvailable() { | ||
| assertThat(VersionGenerator.getSdkVersionNumber()).isNotEqualTo("unknown"); | ||
| assertThat(VersionGenerator.getSdkVersion()).endsWith(VersionGenerator.getSdkVersionNumber()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the intent behind this specific change?
One thing I'm thinking about is, java agent is dependent on autoconfigure, what will the sdkVersionNumber be for a java agent with this change, was that tested? In the case of java agent, is this supposed to reflect the version of the java agent or the autoconfigure "exporter" that it depends on?
autoconfigure in some rare cases can also be used as a standalone.