Initial integration of W3C protocol for incoming request by dhaval24 · Pull Request #776 · microsoft/ApplicationInsights-Java

dhaval24 · 2018-11-29T19:46:58Z

Initial implementation for integration of W3C protocol for Incoming Request.
Related #775 #716

Please note this is not a complete PR yet. There are some tests which are missing and I will add it. Just pushing the code so that, people can take a look and start commenting on approach.

@lmolkova @reyang please review.

For significant contributions please make sure you have completed the following items:

Design discussion issue #
Changes in public surface reviewed
CHANGELOG.md updated

littleaj

Overall this looks great. I posted a few questions/comments.

This would be good to have a smoke test.

littleaj · 2018-11-29T23:45:09Z

+
+        try {
+            if (request == null) {
+                InternalLogger.INSTANCE.error("Failed to resolve correlation. request is null.");


I'm on the fence if these should be error or warning. Since correllation is not necessarliy in every request, I'm leaning towards warning.

I am not fully convinced. I think if the websdk is enabled than correlation should be there by default in every request. Further, if request object is somehow null I think it's justified to have error level log.

littleaj · 2018-11-30T00:09:04Z

+     */
+    private static void addTracestateInResponseHeader(HttpServletResponse response) {
+
+        if (response.containsHeader(CORRELATION_COMPANION_HEADER_NAME)) {


What's the rationale here? Are we opting not to overwrite if something else added the correlation header?

If that's the case, we may want to log something or it could be difficult to determine why correlation isn't working when this evals to true.

That is correct. However looking here, I don't see any case when this would happen. As this is the only method which sets the Header in the response. I think it is safe to remove.

littleaj · 2018-11-30T00:11:49Z

+    public static String retriveTracestate() {
+        //check if context is null - no correlation will happen
+        if (ThreadContext.getRequestTelemetryContext() == null) {
+            InternalLogger.INSTANCE.warn("No correlation wil happen, Thread context is null");


typo: "wil" -> "will"

Also, shouldn't it read "RequestTelemetryContext" instead of "Thread context"?

so the ThreadContext is the wrapper class which stores the RequestTelemetry in InherritableThreadLocal. When the threads change we want to retrive the context from ThreadLocal.

I know, but the ThreadContext (call the static instance T) provides the RequestTelemetryContext (call the instance R)... T.get -> R and this checks if R is null but the messages says T is null.

lmolkova · 2018-11-30T00:17:18Z

            HttpServletResponse response = (HttpServletResponse) res;
-            TelemetryCorrelationUtils.resolveCorrelation(request, response, telemetry);
+            if (isW3CEnabled) {
+                TraceContextCorrelation.resolveCorrelation(request, response, telemetry);


we should be backward compatible and if w3c headers are not present, check for legacy headers

lmolkova · 2018-11-30T00:43:45Z

+                outGoingTraceParent = new Traceparent();
+
+                // represents the id of the current request.
+                requestTelemetry.setId(outGoingTraceParent.getTraceId() + "-" + outGoingTraceParent.getSpanId());


|outGoingTraceParent.getTraceId().outGoingTraceParent.getSpanId().

I know these are backward compatibility comments. Understand it's importance. The question I have is, should we make changes in UX to understand correct format too or should SDK set it as UX expects?
My understanding is that the protocol is what defines the contract between UX and SDK.

if we want to make it work right away - do |outGoingTraceParent.getTraceId().outGoingTraceParent.getSpanId().

there is no UX work planned now to support another contract

lmolkova · 2018-11-30T00:45:04Z

+                    null, incomingTraceparent.getTraceFlags());
+
+                // set id of this request
+                requestTelemetry.setId(outGoingTraceParent.getTraceId() + "-" + outGoingTraceParent.getSpanId());


|outGoingTraceParent.getTraceId().outGoingTraceParent.getSpanId().

lmolkova · 2018-11-30T00:45:58Z

+                requestTelemetry.getContext().getOperation().setId(outGoingTraceParent.getTraceId());
+
+                // represents the parent-id of this request which is combination of traceId and incoming spanId
+                requestTelemetry.getContext().getOperation().setParentId(outGoingTraceParent.getTraceId() + "-" +


|outGoingTraceParent.getTraceId().incomingTraceparent.getSpanId().

lmolkova · 2018-11-30T00:46:35Z

+            ThreadContext.getRequestTelemetryContext().setTracestate(outboundTracestate);
+
+            // Let the callee know the caller's AppId
+            addTracestateInResponseHeader(response);


we should not set tracestate in response header

@lmolkova, what should we pass in the response header to achieve the same functionality? Should we just use the previous header? And why not tracestate?

W3C does not define response headers. we should continue responding with Request-Context to everything

That makes sense. I will update this one. The question comes in mind then what happens when one wants to use OpenCensus SDKs and just change the end-point to go to AI endpoint. This wouldn't work definitely. Do we want to introduce this legacy in the brand new implementation.
I am aware .NET has done this! @reyang what are your thoughts here?
PS : The work is minimal, but it's like once we ship something it's difficult to revert back.

lmolkova · 2018-11-30T00:50:47Z

+                // set parentId as null because this is the is the originating request
+                requestTelemetry.getContext().getOperation().setParentId(null);
+            } else {
+                Traceparent incomingTraceparent = Traceparent.fromString(traceId);


depending on the way you chose to validate - you might need to check incomingTraceparent for null and probably catch exceptions.

If you follow comments I've left in the previous PR (never throw, return brand new traceid and spanid on the invalid parent), this is the right usage, but Traceparent.fromString needs to be fixed to use it like that

Yes, this makes sense. incomingTraceparent can be null and in that case we need to create new TraceId and SpanId. I think it's place resides in Traceparent.fromString(). As I said, there are tests I need to implement :)

lmolkova

RequestTelemtery.Id and ParentId format need to be set in the same way as .NET does it
Backward compatibility: if w3c is configured, but there are not w3c headers, we still need to check for legacy ones (this could be done as separate PR)
do not return tracestate in response

dhaval24 · 2018-11-30T01:18:49Z

@lmolkova @reyang To clarify, does Python and Go also set the request Id and Parent Id in the same format as .NET?

lmolkova · 2018-11-30T01:19:38Z

To clarify, does Python and Go also set the request Id and Parent Id in the same format as .NET?

LocalForwarder does.

dhaval24 · 2018-11-30T03:47:19Z

To clarify, does Python and Go also set the request Id and Parent Id in the same format as .NET?

LocalForwarder does.

Ok, however I don't think most customers even use Local Forwarder as of today (based on telemetry). The direct channel should be as compatible as Local Forwarder.

We can do whatever we want to do and support the UX as of now. However my $0.02 would be, please lets release this as experimental work. UX needs to adhere to the common protocol. That is how essentially systems work. In the final version I would like to see a clean implementation of this protocol which can be treated as a reference.

…d address partial PR comments

dhaval24 · 2018-12-02T05:42:51Z

@reyang @lmolkova @littleaj I have implemented most unit tests to cover W3C integration for Incoming request. I have marked some TODO's where I have questions on what should be desired behavior.
For now, I haven't modified the representation of parentId and Id to match AI protocol. This can be done in separate PR post discussion.

dhaval24 · 2018-12-13T00:58:30Z

Closing this PR in favor of #782 and #785

Initial integration of W3C protocol for incoming request

3afbcea

dhaval24 requested a review from littleaj November 29, 2018 19:47

dhaval24 self-assigned this Nov 29, 2018

dhaval24 added the Enhancement label Nov 29, 2018

littleaj requested review from lmolkova and reyang November 29, 2018 23:06

littleaj reviewed Nov 30, 2018

View reviewed changes

lmolkova reviewed Nov 30, 2018

View reviewed changes

Comment thread ...java/com/microsoft/applicationinsights/web/internal/correlation/TraceContextCorrelation.java Outdated

lmolkova reviewed Nov 30, 2018

View reviewed changes

Comment thread ...java/com/microsoft/applicationinsights/web/internal/correlation/TraceContextCorrelation.java Outdated

lmolkova reviewed Nov 30, 2018

View reviewed changes

Comment thread ...java/com/microsoft/applicationinsights/web/internal/correlation/TraceContextCorrelation.java Outdated

lmolkova reviewed Nov 30, 2018

View reviewed changes

lmolkova suggested changes Nov 30, 2018

View reviewed changes

dhaval24 added 3 commits November 29, 2018 19:47

refactoring header name

abc4fc8

updating the verbosity level when correlation fails

1eae023

Fix bugs in correlation found via tests, implement more unit tests an…

d2f780a

…d address partial PR comments

dhaval24 added 5 commits December 3, 2018 09:17

improve createOutboundTracestate()

5610875

create tracestate when header not available

95b902c

fix test

1ef1ba3

resolve merge conflict

8480736

Refactering code to use Helper methods with TraceContext classes

2a83046

dhaval24 mentioned this pull request Dec 7, 2018

W3C Integration #782

Merged

dhaval24 mentioned this pull request Dec 13, 2018

W3C Final implementation #785

Merged

3 tasks

dhaval24 closed this Dec 13, 2018

trask deleted the W3CIntegration/IncomingRequest branch September 21, 2020 00:17

Conversation

dhaval24 commented Nov 29, 2018

Uh oh!

littleaj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmolkova left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhaval24 commented Nov 30, 2018

Uh oh!

lmolkova commented Nov 30, 2018

Uh oh!

dhaval24 commented Nov 30, 2018

Uh oh!

dhaval24 commented Dec 2, 2018

Uh oh!

dhaval24 commented Dec 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lmolkova left a comment •

edited

Loading