Modify quick pulse code with authentication enabled #1678

kryalama · 2021-05-07T19:05:18Z

Modify quick pulse code to use azure core httppipeline and have aad authentication enabled

heyams · 2021-05-13T19:56:27Z

...c/main/java/com/microsoft/applicationinsights/internal/authentication/AadAuthentication.java

+
+    public static void init(AuthenticationType authenticationType, String clientId, String keePassDatabasePath,
+                            String tenantId, String clientSecret, String authorityHost) {
+        AadAuthentication.instance = new AadAuthentication(authenticationType, clientId, keePassDatabasePath, tenantId,


synchronized block?

i know this will get called only once.. do we still need to prevent user from creating a new instance of AadAuthentication each time when init is called?

...c/main/java/com/microsoft/applicationinsights/internal/authentication/AadAuthentication.java

heyams · 2021-05-13T20:00:45Z

.../main/java/com/microsoft/applicationinsights/internal/authentication/AuthenticationType.java

+package com.microsoft.applicationinsights.internal.authentication;
+
+public enum AuthenticationType {
+    UAMI, SAMI, INTELLIJ, VSCODE, CLIENTSECRET


Can we document what each type is?

link to the azure doc will be helpful.

lgtm-com · 2021-05-13T22:25:52Z

This pull request introduces 1 alert when merging 503582d into 83b96b6 - view on LGTM.com

new alerts:

1 for Missing space in string literal

lgtm-com · 2021-05-15T04:05:59Z

This pull request introduces 1 alert when merging 9c17075 into 83b96b6 - view on LGTM.com

new alerts:

1 for Missing space in string literal

lgtm-com · 2021-05-15T06:44:42Z

This pull request introduces 1 alert when merging d123841 into 83b96b6 - view on LGTM.com

new alerts:

1 for Missing space in string literal

trask

👍

...m/microsoft/applicationinsights/agent/internal/wasbootstrap/configuration/Configuration.java

...c/main/java/com/microsoft/applicationinsights/internal/authentication/AadAuthentication.java

trask · 2021-05-17T20:41:13Z

core/src/main/java/com/microsoft/applicationinsights/TelemetryClient.java

-
+        // handle AAD authentication
+        // TODO handle authentication exceptions
+        HttpPipelinePolicy authenticationPolicy = AadAuthentication.getInstance().getAuthenticationPolicy();


won't this fail auth not configured? getInstance() throws exception?

Yes if we call init() only when authentication is enabled. for now I removed the auth enabled check to create the authentication object. But we will end up with object with nullable variables.

...ing/src/main/java/com/microsoft/applicationinsights/agent/internal/AiComponentInstaller.java

...c/main/java/com/microsoft/applicationinsights/internal/authentication/AadAuthentication.java

lgtm-com · 2021-05-18T22:05:15Z

This pull request introduces 1 alert when merging 8602098 into 1ffad2b - view on LGTM.com

new alerts:

1 for Missing space in string literal

trask

did u mean to update the submodule in this PR?

...nt-tooling/src/main/java/com/microsoft/applicationinsights/agent/internal/AppIdSupplier.java

...m/microsoft/applicationinsights/agent/internal/wasbootstrap/configuration/Configuration.java

...c/main/java/com/microsoft/applicationinsights/internal/authentication/AadAuthentication.java

lgtm-com · 2021-05-19T17:41:57Z

This pull request introduces 1 alert when merging 857c29f into 1ffad2b - view on LGTM.com

new alerts:

1 for Missing space in string literal

lgtm-com · 2021-05-19T23:17:13Z

This pull request introduces 1 alert when merging 36d6e7b into 1ffad2b - view on LGTM.com

new alerts:

1 for Missing space in string literal

trask · 2021-05-19T23:20:47Z

@kryalama can you fix the lgtm bot alert above?

kryalama · 2021-05-19T23:25:03Z

@kryalama can you fix the lgtm bot alert above?

Yes will do. Also looking at the failing smoke test.

...mExit/src/smokeTest/java/com/microsoft/applicationinsights/smoketest/SpringBootAutoTest.java

…t/applicationinsights/smoketest/SpringBootAutoTest.java

heyams · 2021-05-21T01:31:02Z

...main/java/com/microsoft/applicationinsights/internal/channel/common/LazyAzureHttpClient.java

+            try {
+                delegate = init();
+            } catch (RuntimeException e) {
+                initException = e;


i am curious to know why not throw here directly and wait for second called of this method then throw at line 42?

trask added 30 commits April 25, 2021 14:42

Temporarily add to this repo

de936a1

Bump version to not conflict

fc61795

Add azure-monitor-opentelemetry-exporter dependency

a3eacc9

Remove tests

7daedb2

Remove create default telemetry configuration

8fac403

Replace Channel with Rest Client

dcef630

Remove internal.channel package

800fb8c

More

f61986a

Update lockfiles

07c7f8b

Core compiling

1f2fdc5

Core tests passing

cfaffdb

Remove Exporter

6944be2

Unit tests passing (still some ignored tho)

ae6b4e7

Remove more

125873e

Fix some FIXMEs

6a671b7

Fix more FIXMEs

08150df

Fix more FIXMEs

bc77fb9

Bump version to 3.1.0

69175d3

Some fixes

e53cdac

Not using external exporter for now

ce51afe

Need to use BatchSpanProcessor now

82e337b

Need to upstream: fix double slash in track url

a537f46

Need to upstream: remove statusDescription property

529bddb

DO NOT MERGE, TOO VERBOSE

0753eea

Need to upstream: fix formatted duration

4c11910

Optimization

45c1cc0

More

df13221

Update gradle dependencies from compile to impl

5d49601

Bring back old exporter

e47fec6

Update old exporter

40f1ed1

heyams reviewed May 13, 2021

View reviewed changes

address comments, fix spotbug issues

503582d

fix smoke test, add lazyazurehttpclient, address comments

9c17075

fix spotbug issues

d123841

trask reviewed May 17, 2021

View reviewed changes

kryalama added 2 commits May 17, 2021 21:37

address comments

ce129f1

resolve merge conflicts

8602098

kryalama requested a review from trask May 18, 2021 21:38

trask reviewed May 18, 2021

View reviewed changes

trask added 2 commits May 19, 2021 09:56

Fix submodule

7e82ede

Fix submodule

857c29f

fix cds profiler issue, address comments

d796859

kryalama requested a review from trask May 19, 2021 21:57

Remove a couple of null checks

36d6e7b

kryalama added 2 commits May 20, 2021 13:30

fix lgtm issue

6d5df1d

spotbug workaround, ignore systemexit test for now

05bc3e4

trask reviewed May 20, 2021

View reviewed changes

...mExit/src/smokeTest/java/com/microsoft/applicationinsights/smoketest/SpringBootAutoTest.java Outdated Show resolved Hide resolved

Update test/smoke/testApps/SystemExit/src/smokeTest/java/com/microsof…

d0f088b

…t/applicationinsights/smoketest/SpringBootAutoTest.java

trask approved these changes May 20, 2021

View reviewed changes

heyams reviewed May 21, 2021

View reviewed changes

kryalama merged commit 934bc7b into aad May 21, 2021

trask deleted the kryalama/quickpulseaad branch May 26, 2021 02:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify quick pulse code with authentication enabled #1678

Modify quick pulse code with authentication enabled #1678

kryalama commented May 7, 2021

heyams May 13, 2021

heyams May 13, 2021

heyams May 13, 2021

heyams May 13, 2021

lgtm-com bot commented May 13, 2021

lgtm-com bot commented May 15, 2021

lgtm-com bot commented May 15, 2021

trask left a comment

trask May 17, 2021

kryalama May 18, 2021

lgtm-com bot commented May 18, 2021

trask left a comment

lgtm-com bot commented May 19, 2021

lgtm-com bot commented May 19, 2021

trask commented May 19, 2021

kryalama commented May 19, 2021

heyams May 21, 2021

Modify quick pulse code with authentication enabled #1678

Modify quick pulse code with authentication enabled #1678

Conversation

kryalama commented May 7, 2021

heyams May 13, 2021

Choose a reason for hiding this comment

heyams May 13, 2021

Choose a reason for hiding this comment

heyams May 13, 2021

Choose a reason for hiding this comment

heyams May 13, 2021

Choose a reason for hiding this comment

lgtm-com bot commented May 13, 2021

lgtm-com bot commented May 15, 2021

lgtm-com bot commented May 15, 2021

trask left a comment

Choose a reason for hiding this comment

trask May 17, 2021

Choose a reason for hiding this comment

kryalama May 18, 2021

Choose a reason for hiding this comment

lgtm-com bot commented May 18, 2021

trask left a comment

Choose a reason for hiding this comment

lgtm-com bot commented May 19, 2021

lgtm-com bot commented May 19, 2021

trask commented May 19, 2021

kryalama commented May 19, 2021

heyams May 21, 2021

Choose a reason for hiding this comment