vnext, add context to client #1067

weidongxu-microsoft · 2020-03-03T03:59:09Z

Proposed implementation to add context for UserAgentPolicy

Common AzureServiceClient fill common part, OS, Java version.
Client fill mgmt client name.
Method fill api version.

weidongxu-microsoft · 2020-03-03T03:59:55Z

Let me know your opinion before I start implement the generator.

ChenTanyi · 2020-03-03T06:36:48Z

...-mgmt-resources/src/main/java/com/azure/management/resources/models/ResourceGroupsInner.java

-        return service.get(this.client.getHost(), resourceGroupName, this.client.getSubscriptionId(), this.client.getApiVersion());
+        return FluxUtil
+                .withContext(context -> service.get(this.client.getHost(), resourceGroupName, this.client.getSubscriptionId(), this.client.getApiVersion(), context))
+                .subscriberContext(context -> context.putAll(FluxUtil.toReactorContext(this.client.getContext().addData("Sdk-Version", this.client.getApiVersion()))));


In https://azure.github.io/azure-sdk/general_azurecore.html#telemetry-policy, the Sdk-Version is the version of sdk (1.31.0 etc.) instead of the api -version. For Sdk-Name, I think it should use the whole package name (azure.management.resources).

Got it. Sdk-Name seems not the whole package name (storage.blobs is apparently not a package name).

Then I will leave both to be set from the builder (similar to host/subscriptionId, manager could set this part of the Context).

I've tested for azure-storage-blob, 12.4.0, it use this whole artifactId in user-agent. I'm not sure if all the data-plane teams use it in user-agent.

is it necessary or ok to track machine id?

I think there is no machine id? Just OS name/version, package name/version, java version.

weidongxu-microsoft · 2020-03-12T03:42:25Z

Intended for #1090

Context in LRO seems not working correctly.

weidongxu-microsoft · 2020-03-24T01:52:44Z

azure-mgmt-resources/src/main/java/com/azure/management/AzureServiceClient.java

+        if (sdkName == null) {
+            String packageName = this.getClass().getPackage().getName();
+            if (packageName.endsWith(".models")) {
+                sdkName = packageName.substring(0, packageName.length() - ".models".length());


I am doing a bit of trick here.

ComputeManagementClientImpl from com.azure.management.compute.models would have Sdk-Name com.azure.management.compute

weidongxu-microsoft · 2020-03-25T04:31:04Z

@ChenTanyi @xccc-msft @yungezz

Let me know if there is anything need discussion.

Sample user-agent for now (2.0.0 is hardcoded, this.getClass().getPackage().getImplementationVersion() does not work in debug).

User-Agent:azsdk-java-com.azure.management.resources/2.0.0 (13.0.1; Windows 10 10.0)

f5a3179 is regen of resources with context enabled.

weidongxu-microsoft · 2020-03-25T04:33:27Z

As mentioned, the context for LRO is not solved yet. So only first request to it had context.

yungezz · 2020-03-25T13:43:04Z

@weidongxu-microsoft what's v1 useragent looks like? how to distinguish them in future? by version or by namespace?

yungezz

need to setContext in each service? what's the lowest layer it could be added so upper layer don't need to care about it

yungezz · 2020-03-25T13:45:27Z

azure-mgmt-resources/src/main/java/com/azure/management/AzureServiceClient.java

+            }
+        }
+        context = context.addData("Sdk-Name", sdkName);
+        context = context.addData("Sdk-Version", SDK_VERSION);


I remember there's limitation on lenght of userAgent at different service, such as cosmos. how long is length of our fixed useragent string?

A typical one is such
azsdk-java-com.azure.management.resources/2.0.0 (13.0.1; Windows 10 10.0)

Since user can set ApplicatinId, there is no limit on the length. Though we can truncate that ApplicationId if necessary. @ChenTanyi
https://github.com/Azure/azure-libraries-for-java/blob/vnext/azure-mgmt-resources/src/main/java/com/azure/management/UserAgentPolicy.java#L112-L117

track1 is not much to compare since Azure SDK gives new guideline for it.
https://azure.github.io/azure-sdk/general_azurecore.html#telemetry-policy

For reference track1 Azure-SDK-For-Java/null OS:Windows 10/10.0 MacAddressHash:9e21c3cc3783e949641d36ffd6a2e0c027267de76b62dcc951a57438fbc15f36 Java:13.0.1 (ResourceManagementClient, 2019-08-01)

thanks Weidong for detail info. According to azure core tememetry, shouldn't sdk package name and version set by azure.core?

@tanyi "DO enforce that the application ID is no more than 24 characters in length. " from azure core guideline, again, shouldn't this be done in azure.core/azure.core.mgmt?

@yungezz The azure core has a User-Agent policy, but it could only send constant UA, we have raised an issue and pr about it. But the azure-core team advise us to implement out own. Meanwhile, I think currently all code in com.azure.management could moved into azure-core-mgmt.

Personally I do not see how azure.core could get sdk package name and sdk version without we setting it to azure.core in the first place. Maybe I misunderstood the question.

so package-name, pacakge-version isn't in the constant UA? then that makes a lot of sense. Thanks both to help clarify!

These are in the UA, but it cannot be changed between request. That
is not we need. We use a restclient for every package. So we need to change UA during request. https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/UserAgentPolicy.java#L84

ChenTanyi

I think it is ok except for LRO currently.

weidongxu-microsoft · 2020-03-27T05:34:19Z

@yungezz

In my understanding, Context need to be set to each REST API call. This provides finest grain for user. The challenge is to provide user other levels of control consistently (e.g. global, per service, per cluster of requests e.g. those for doing one task requires a group of resources).

Context is used for UserAgent, and Tracing, but it is more than that.

Here the code is in parent class of ManagementClient, so this is one shared piece for all REST API.

yungezz · 2020-03-27T12:33:10Z

@yungezz

In my understanding, Context need to be set to each REST API call. This provides finest grain for user. The challenge is to provide user other levels of control consistently (e.g. global, per service, per cluster of requests e.g. those for doing one task requires a group of resources).

Context is used for UserAgent, and Tracing, but it is more than that.

Here the code is in parent class of ManagementClient, so this is one shared piece for all REST API.

Normally SDK could expose a setUserAgent() interface which depends on service(some service exposing setUserAgent(), some not) to append user customized userAgent to system default ones, which includes context from different layers: service, SDK, user customized etc.

The PR itself looks good, since it's our general context.

weidongxu-microsoft · 2020-03-30T02:23:41Z

For context and tracing, I think I am referring to these possible use cases. https://github.com/Azure/azure-sdk-for-java/tree/master/sdk/core/azure-core-tracing-opentelemetry#examples, which is user's own tracing.

UserAgent appears to be our tracing (with or without user's knowledge).

draft, add context to client

13605d5

weidongxu-microsoft requested review from yaohaizh, xseeseesee and ChenTanyi March 3, 2020 03:59

weidongxu-microsoft changed the title ~~vraft, add context to client~~ vnext, draft, add context to client Mar 3, 2020

npe protection

940aed4

ChenTanyi reviewed Mar 3, 2020

View reviewed changes

weidongxu-microsoft added 4 commits March 3, 2020 15:36

config for generator to add context

ac9a439

minor fix

d3c80e2

Merge branch 'vnext' into vnext_draft_context

feedd1a

use getContext directly from AzureServiceClient

cd5528d

cleanup

71a5804

weidongxu-microsoft changed the title ~~vnext, draft, add context to client~~ vnext, add context to client Mar 24, 2020

weidongxu-microsoft marked this pull request as ready for review March 24, 2020 01:49

Merge branch 'vnext' into vnext_draft_context

1b6e111

weidongxu-microsoft commented Mar 24, 2020

View reviewed changes

weidongxu-microsoft added 2 commits March 25, 2020 12:27

add docstring

f7319f2

Merge branch 'vnext' into vnext_draft_context

6b90803

weidongxu-microsoft added 3 commits March 25, 2020 12:35

regen resources with context enabled

87d8e8c

use flatten=1

4adcf6d

remove cloud error

bf85e7f

yungezz reviewed Mar 25, 2020

View reviewed changes

ChenTanyi approved these changes Mar 26, 2020

View reviewed changes

yungezz approved these changes Mar 27, 2020

View reviewed changes

weidongxu-microsoft mentioned this pull request Mar 30, 2020

shared/default Context in AzureServiceClient Azure/azure-sdk-for-java#9667

Merged

weidongxu-microsoft force-pushed the vnext_draft_context branch from 8672b74 to bf85e7f Compare April 10, 2020 02:31

weidongxu-microsoft force-pushed the vnext branch from 90ea1f1 to 6ebeb19 Compare April 10, 2020 03:10

weidongxu-microsoft closed this Apr 10, 2020

weidongxu-microsoft deleted the vnext_draft_context branch April 10, 2020 03:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vnext, add context to client #1067

vnext, add context to client #1067

weidongxu-microsoft commented Mar 3, 2020

weidongxu-microsoft commented Mar 3, 2020

ChenTanyi Mar 3, 2020

weidongxu-microsoft Mar 3, 2020

ChenTanyi Mar 3, 2020

yungezz Mar 12, 2020

weidongxu-microsoft Mar 12, 2020

weidongxu-microsoft commented Mar 12, 2020

weidongxu-microsoft Mar 24, 2020

weidongxu-microsoft commented Mar 25, 2020 •

edited

Loading

weidongxu-microsoft commented Mar 25, 2020 •

edited

Loading

yungezz commented Mar 25, 2020

yungezz left a comment •

edited

Loading

yungezz Mar 25, 2020

weidongxu-microsoft Mar 25, 2020 •

edited

Loading

yungezz Mar 29, 2020

yungezz Mar 29, 2020

ChenTanyi Mar 30, 2020

weidongxu-microsoft Mar 30, 2020 •

edited

Loading

yungezz Mar 30, 2020

ChenTanyi Mar 30, 2020

ChenTanyi left a comment

weidongxu-microsoft commented Mar 27, 2020 •

edited

Loading

yungezz commented Mar 27, 2020

weidongxu-microsoft commented Mar 30, 2020 •

edited

Loading

vnext, add context to client #1067

vnext, add context to client #1067

Conversation

weidongxu-microsoft commented Mar 3, 2020

weidongxu-microsoft commented Mar 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weidongxu-microsoft commented Mar 12, 2020

Choose a reason for hiding this comment

weidongxu-microsoft commented Mar 25, 2020 • edited Loading

weidongxu-microsoft commented Mar 25, 2020 • edited Loading

yungezz commented Mar 25, 2020

yungezz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weidongxu-microsoft Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weidongxu-microsoft Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChenTanyi left a comment

Choose a reason for hiding this comment

weidongxu-microsoft commented Mar 27, 2020 • edited Loading

yungezz commented Mar 27, 2020

weidongxu-microsoft commented Mar 30, 2020 • edited Loading

weidongxu-microsoft commented Mar 25, 2020 •

edited

Loading

weidongxu-microsoft commented Mar 25, 2020 •

edited

Loading

yungezz left a comment •

edited

Loading

weidongxu-microsoft Mar 25, 2020 •

edited

Loading

weidongxu-microsoft Mar 30, 2020 •

edited

Loading

weidongxu-microsoft commented Mar 27, 2020 •

edited

Loading

weidongxu-microsoft commented Mar 30, 2020 •

edited

Loading