Support HTTP based Azure SDK service tracing by Spring Sleuth implementation #24192

moarychan · 2021-09-17T10:59:51Z

You can find the feature proposal here, this PR will support the service HTTP based, the tracer is delegated to Spring Sleuth, it's available to record the service activities when enabling the Spring Sleuth function.

check-enforcer · 2021-09-28T01:28:31Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run java - [service] - ci

moarychan · 2021-09-28T03:28:54Z

/azp run java - spring - tests

azure-pipelines · 2021-09-28T03:29:07Z

Azure Pipelines successfully started running 1 pipeline(s).

stliu · 2021-09-28T06:03:48Z

...pring-cloud-trace-sleuth/src/main/java/com/azure/spring/tracing/sleuth/SleuthHttpPolicy.java

+    private final Propagator propagator;
+
+    // standard attributes with http call information
+    private static final String HTTP_USER_AGENT = "http.user_agent";


https://github.com/Azure/azure-sdk/blob/main/docs/tracing/distributed-tracing-conventions.yml

could you also take a look of this doc, let's make the tracing id aligned with the doc

also, which stage will this policy be added into pipeline, before or after retry? will retry requests be tracked?

this policy will be placed after a HttpPipelinePosition.PER_RETRY by default, it uses the default pipeline position,

azure-sdk-for-java/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HttpPipelinePolicy.java

Line 37 in ed7d893

return HttpPipelinePosition.PER_RETRY;

the retry request will be tracked.

stliu · 2021-09-28T06:08:03Z

...trace-sleuth/src/main/java/com/azure/spring/tracing/sleuth/implementation/HttpTraceUtil.java

+
+public final class HttpTraceUtil {
+
+    private static final String STATUS_100 = "Continue";


I don't think you need to define these stuff by youslef, how about using something existing, like org.springframework.http.HttpStatus

OK, the dependency org.springframework:spring-web will be added.

stliu · 2021-09-28T06:08:56Z

...-cloud-trace-sleuth/src/main/java/com/azure/spring/tracing/sleuth/implementation/SpanId.java

+ * </ul>
+ */
+@Immutable
+public final class SpanId {


this is no such class within spring-cloud-sleuth project?

Yes, not found a suitable class.

moarychan · 2021-09-28T10:36:56Z

/azp run java - spring - tests

azure-pipelines · 2021-09-28T10:37:10Z

Azure Pipelines successfully started running 1 pipeline(s).

moarychan · 2021-09-28T12:52:03Z

/azp run java - spring - tests

saragluna · 2021-10-19T03:11:38Z

sdk/spring/pom.xml

@@ -194,6 +200,7 @@
        <module>spring-cloud-azure-starter-stream-servicebus-queue</module>
        <module>spring-cloud-azure-stream-binder-servicebus-topic</module>
        <module>spring-cloud-azure-starter-stream-servicebus-topic</module>
+        <module>spring-cloud-azure-trace-sleuth</module>


nit: move this after spring-cloud-azure-stream-binder-test

saragluna · 2021-10-19T03:11:44Z

sdk/spring/pom.xml

@@ -239,6 +246,7 @@
        <module>spring-cloud-azure-starter-stream-servicebus-queue</module>
        <module>spring-cloud-azure-stream-binder-servicebus-topic</module>
        <module>spring-cloud-azure-starter-stream-servicebus-topic</module>
+        <module>spring-cloud-azure-trace-sleuth</module>


saragluna · 2021-10-19T03:12:23Z

...ain/java/com/azure/spring/cloud/autoconfigure/trace/sleuth/AzureSleuthAutoConfiguration.java

+/**
+ * Auto-configuration for an Azure SDK Sleuth {@link Tracer}.
+ */
+@Role(BeanDefinition.ROLE_INFRASTRUCTURE)


Why adding this?

I thought there would be the following confusing log:
trationDelegate$BeanPostProcessorChecker : Bean 'com.azure.spring.cloud.autoconfigure.trace.sleuth.AzureSleuthAutoConfiguration' of type [com.azure.spring.cloud.autoconfigure.trace.sleuth.AzureSleuthAutoConfiguration] is not eligible for getting processed by all BeanPostProcessors (for example: not eligible for auto-proxying).
After added this @ROLE, it looks good.

saragluna · 2021-10-19T03:12:42Z

sdk/spring/spring-cloud-azure-autoconfigure/pom.xml

+    <dependency>
+      <groupId>org.springframework.cloud</groupId>
+      <artifactId>spring-cloud-sleuth-autoconfigure</artifactId>
+      <version>3.0.3</version> <!-- {x-version-update;org.springframework.cloud:spring-cloud-sleuth-autoconfigure;external_dependency} -->
+      <optional>true</optional>
+    </dependency>
+


Why do we need this?

There's a @ConditionOnClass org.springframework.cloud.sleuth.autoconfig.SleuthTracerProperties in AzureSleuthAutoConfiguration.

saragluna · 2021-10-19T03:14:31Z

...ain/java/com/azure/spring/cloud/autoconfigure/trace/sleuth/AzureSleuthAutoConfiguration.java

+ */
+@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
+@Configuration(proxyBeanMethods = false)
+@ConditionalOnClass({ SleuthHttpPolicy.class, Tracer.class, SleuthTracerProperties.class })


Do we need to depend on the auto-configure of sleuth? Or is it enough if we find a Tracer bean?

Actually, it's not required, the sleuth API Tracer class is enough.

saragluna · 2021-10-19T03:17:06Z

.../java/com/azure/spring/core/trace/sleuth/AzureHttpClientBuilderFactoryBeanPostProcessor.java

+    @Override
+    @SuppressWarnings({ "rawtypes", "unchecked" })
+    public Object postProcessAfterInitialization(Object bean, String beanName) throws BeansException {
+        if (beanFactory != null && bean instanceof AbstractAzureHttpClientBuilderFactory) {


will the beanfactory be null?

Due to @nullable, it can be remove too.

saragluna · 2021-10-19T03:18:00Z

sdk/spring/spring-cloud-azure-trace-sleuth/pom.xml

+      <groupId>com.azure</groupId>
+      <artifactId>azure-storage-blob</artifactId>
+      <version>12.14.1</version>  <!-- {x-version-update;com.azure:azure-storage-blob;dependency} -->
+      <scope>test</scope>
+    </dependency>


Why adding this storage blob dependency?

It's for test blob service builder

saragluna · 2021-10-19T03:18:50Z

sdk/spring/spring-cloud-azure-trace-sleuth/pom.xml

+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-jar-plugin</artifactId>
+        <version>3.1.2</version> <!-- {x-version-update;org.apache.maven.plugins:maven-jar-plugin;external_dependency} -->
+        <configuration>
+          <archive>
+            <manifest>
+              <addDefaultImplementationEntries>true</addDefaultImplementationEntries>
+            </manifest>
+          </archive>
+        </configuration>
+      </plugin>
+    </plugins>
+  </build>


What's this used for?

I copied it from the core module, will remove it.

saragluna · 2021-10-19T03:20:05Z

...cloud-azure-trace-sleuth/src/main/java/com/azure/spring/tracing/sleuth/SleuthHttpPolicy.java

+    @Override
+    public int getOrder() {
+        return Ordered.LOWEST_PRECEDENCE;
+    }


Why adding this Ordered?

I thought this bean should be the low precedence to register, after testing, it can be removed.

saragluna · 2021-10-19T03:20:37Z

...cloud-azure-trace-sleuth/src/main/java/com/azure/spring/tracing/sleuth/SleuthHttpPolicy.java

+        if ((boolean) context.getData(DISABLE_TRACING_KEY).orElse(false)) {
+            return next.process();
+        }
+        //        tracer.getBaggage()


do we need this comment?

moarychan · 2021-10-19T11:16:47Z

/azp run java - spring - tests

azure-pipelines · 2021-10-19T11:17:16Z

Azure Pipelines successfully started running 1 pipeline(s).

…re-sdk-for-java into feature/azure-trace-sleuth-implementation

moarychan · 2021-10-21T10:43:25Z

/azp run java - spring - tests

azure-pipelines · 2021-10-21T10:43:38Z

Azure Pipelines successfully started running 1 pipeline(s).

lmolkova · 2021-10-25T22:41:22Z

Hey @moarychan and @saragluna I'm doing some changes in #25012 related to Tracer. I believe you can ignore the rest of that PR, as it's OTel-specific, but the important difference is on PARENT_SPAN_KEY:
Span is too narrow and we'd want to propagate more than that: In OTel it's Context that contains span + baggage + possible other markers. Sleuth (brave) doesn't have context and passes baggage + spans separately.

What I propose: let's use the same wider name (e.g. PARENT_TRACE_CONTEXT_KEY) for Otel context or Sleuth spans and always refer to it in the docs, but deprecate PARENT_SPAN_KEY. I'm open to any other names if you have any suggestions.

lmolkova

The code looks fine, I left some minor comments.

The only blocker I see is docs: It's important to highlight that Sleuth integration is a solution that applies to existing Sleuth users and has limited functionality (i.e. HTTP requests tracing only). Microsoft supports OpenTelemetry company-wide and we should point users in this direction to provide a better long-term experience to users and avoid any reputational damage.

lmolkova · 2021-10-25T22:44:08Z

...cloud-azure-trace-sleuth/src/main/java/com/azure/spring/tracing/sleuth/SleuthHttpPolicy.java

+        // run the next policy and handle success and error
+        return next.process()
+                   .doOnEach(SleuthHttpPolicy::handleResponse)
+                   .contextWrite(Context.of("TRACING_SPAN", span, "REQUEST", request));


nit: is "REQUEST" needed?

lmolkova · 2021-10-25T22:46:46Z

...cloud-azure-trace-sleuth/src/main/java/com/azure/spring/tracing/sleuth/SleuthHttpPolicy.java

+            return next.process();
+        }
+
+        Span parentSpan = (Span) context.getData(PARENT_SPAN_KEY).orElse(tracer.currentSpan());


It might make sense to move span creation to contextWrite similarly to

azure-sdk-for-java/sdk/core/azure-core-tracing-opentelemetry/src/main/java/com/azure/core/tracing/opentelemetry/OpenTelemetryHttpPolicy.java

Line 99 in d304bb9

.contextWrite(reactor.util.context.Context.of(REACTOR_PARENT_TRACE_CONTEXT_KEY, startSpan(context)));

in #25012

It ensures that span is created when someone really subscribes to this reactor call and not on the hot path.

lmolkova · 2021-10-25T22:48:10Z

...cloud-azure-trace-sleuth/src/main/java/com/azure/spring/tracing/sleuth/SleuthHttpPolicy.java

+            request.getHeaders().getValue("User-Agent"));
+        putTagIfNotEmptyOrNull(span, HTTP_METHOD, request.getHttpMethod().toString());
+        putTagIfNotEmptyOrNull(span, HTTP_URL, request.getUrl().toString());
+        Optional<Object> tracingNamespace = context.getData(AZ_TRACING_NAMESPACE_KEY);


these are Otel conventions. Brave conventions are close, but not the same: https://github.com/openzipkin/brave/blob/master/instrumentation/http/README.md

lmolkova · 2021-10-25T22:49:28Z

...cloud-azure-trace-sleuth/src/main/java/com/azure/spring/tracing/sleuth/SleuthHttpPolicy.java

+                requestId = response.getHeaderValue(REQUEST_ID);
+            }
+
+            putTagIfNotEmptyOrNull(span, REQUEST_ID, requestId);


this has been changed slightly here to be consistent across all languages 7a5d4ab#diff-74209afca0d9d5d4a8a40b7b0e0e28b0df6529e0e2af5c4fff1b33cc54ff4b24

lmolkova · 2021-10-25T22:50:43Z

...trace-sleuth/src/main/java/com/azure/spring/tracing/sleuth/implementation/HttpTraceUtil.java

+        // Error status, try to parse the error status.
+        HttpStatus status = HttpStatus.resolve(statusCode);
+        if (status != null) {
+            return span.tag("http.status_message", status.getReasonPhrase());


please use http.status_code per brave conventions https://github.com/openzipkin/brave/blob/master/instrumentation/http/README.md

Not sure how to modify it, please help provide more specific information.

lmolkova · 2021-10-25T23:04:52Z

sdk/spring/spring-cloud-azure-trace-sleuth/README.md

+[Source code][src] | [Package (Maven)][package] | [API reference documentation][refdocs]
+
+## Getting started
+


Since Microsoft embraces OpenTelemetry, it's important to highlight here that it's the short term solution targeting existing Sleuth users with manual instrumentation.

I would suggest putting something along the lines of:

"Spring Cloud for Azure Sleuth Trace is the solution for users who manually instrumented their applications with Sleuth and are looking to add observability into HTTP requests made by Azure SDKs.

For everyone else, we recommend using OpenTelemetry as a ubiquitous and portable observability solution for tracing, metrics, and logs supported by most APM tools with extensive integrations across Java ecosystem"

moarychan · 2021-10-29T10:35:06Z

Hi @lmolkova , Thank you so much for your comments.
The new keys from your PR are fine, but now I can not import using directly, once the azure-core package is released, I will make a sync update.

lmolkova · 2021-11-01T17:47:21Z

Hi @lmolkova , Thank you so much for your comments. The new keys from your PR are fine, but now I can not import using directly, once the azure-core package is released, I will make a sync update.

no problem, you can keep PARENT_SPAN_KEY for as long as you want - it's deprecated, but stays for a while. And azure-core is backward compatible with it

moarychan · 2021-11-17T07:05:41Z

/azp run java - spring - tests

azure-pipelines · 2021-11-17T07:05:54Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost added the azure-spring All azure-spring related issues label Sep 17, 2021

moarychan changed the title ~~Support Azure tracing by Sleuth implementation~~ Support Azure SDK service tracing by Spring Sleuth implementation Sep 17, 2021

moarychan changed the title ~~Support Azure SDK service tracing by Spring Sleuth implementation~~ Support HTTP based Azure SDK service tracing by Spring Sleuth implementation Sep 28, 2021

This was unlinked from issues Sep 28, 2021

[FEATURE REQ] The SDK Service Bus service builder can not use a custom TraceProvider #24085

Closed

[FEATURE REQ] The SDK Event Hubs service client builder can not use a custom TraceProvider #24086

Closed

moarychan marked this pull request as ready for review September 28, 2021 05:38

moarychan requested review from alzimmermsft, backwind1233, chenrujun, g2vinay, JonathanGiles, michaelqi793, samvaity, saragluna, stliu, yiliuTo, zhichengliu12581 and ZhuXiaoBing-cn as code owners September 28, 2021 05:38

stliu reviewed Sep 28, 2021

View reviewed changes

moarychan requested review from vcolin7, vxfield, weidongxu-microsoft, weshaggard, xinlian12 and a team as code owners October 15, 2021 10:03

moarychan marked this pull request as draft October 18, 2021 01:49

Sleuth implementation for sdk http policy

0ec6837

moarychan force-pushed the feature/azure-trace-sleuth-implementation branch from 2af1c96 to 0ec6837 Compare October 18, 2021 07:23

saragluna requested changes Oct 19, 2021

View reviewed changes

fix comments

71a707b

Moary Chen added 2 commits October 21, 2021 14:26

Merge branch 'feature/azure-spring-cloud-4.0' of github.com:Azure/azu…

ac9b042

…re-sdk-for-java into feature/azure-trace-sleuth-implementation

Add tracing dependency to bom management

26bf22d

lmolkova mentioned this pull request Oct 23, 2021

Fix OpenTelemetry context propagation and duplication issues #25012

Merged

lmolkova requested changes Oct 25, 2021

View reviewed changes

fix comments

ef7e148

Merge feature/4.0 branch

baff290

moarychan marked this pull request as ready for review November 17, 2021 01:12

Merge branch 'feature/azure-spring-cloud-4.0'

a5e39e5

saragluna approved these changes Nov 17, 2021

View reviewed changes

moarychan merged commit efdde62 into Azure:feature/azure-spring-cloud-4.0 Nov 17, 2021


		public final class HttpTraceUtil {

		private static final String STATUS_100 = "Continue";

		[Source code][src] \| [Package (Maven)][package] \| [API reference documentation][refdocs]

		## Getting started

Support HTTP based Azure SDK service tracing by Spring Sleuth implementation #24192

Support HTTP based Azure SDK service tracing by Spring Sleuth implementation #24192

Conversation

moarychan commented Sep 17, 2021 • edited Loading

check-enforcer bot commented Sep 28, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

moarychan commented Sep 28, 2021

azure-pipelines bot commented Sep 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moarychan Sep 28, 2021 • 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

Choose a reason for hiding this comment

moarychan commented Sep 28, 2021

azure-pipelines bot commented Sep 28, 2021

moarychan commented Sep 28, 2021

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moarychan commented Oct 19, 2021

azure-pipelines bot commented Oct 19, 2021

moarychan commented Oct 21, 2021

azure-pipelines bot commented Oct 21, 2021

lmolkova commented Oct 25, 2021

lmolkova left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmolkova Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

moarychan commented Oct 29, 2021

lmolkova commented Nov 1, 2021

moarychan commented Nov 17, 2021

azure-pipelines bot commented Nov 17, 2021

moarychan commented Sep 17, 2021 •

edited

Loading

moarychan Sep 28, 2021 •

edited

Loading

lmolkova Oct 25, 2021 •

edited

Loading