W3C : Tracecontext classes #775

dhaval24 · 2018-11-28T19:25:52Z

This PR brings in the required classes for integrating W3C correlation protocol.
Related #716

@reyang @lmolkova take a look.

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

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

...ava/com/microsoft/applicationinsights/web/internal/correlation/tracecontext/Traceparent.java

...java/com/microsoft/applicationinsights/web/internal/correlation/tracecontext/Tracestate.java

reyang · 2018-11-28T19:30:02Z

...java/com/microsoft/applicationinsights/web/internal/correlation/tracecontext/Tracestate.java

+ *
+ * Implementations can add vendor specific details here.
+ *
+ * @author Reiley Yang


Curious, why do we put author name here?

Last time I've seen this was when I work on the C++ compiler, at that time there was no "git blame".

Most Java libraries do this :) Its a common pattern in Java projects.

...ava/com/microsoft/applicationinsights/web/internal/correlation/tracecontext/Traceparent.java

reyang

Looks good for now.

reyang · 2018-11-28T19:59:02Z

Please merge this to a working branch (e.g. w3c) instead of master.

reyang

We should not merge into master until feature complete.
Please create a feature branch and merge to it.

littleaj · 2018-11-28T19:49:11Z

...ava/com/microsoft/applicationinsights/web/internal/correlation/tracecontext/Traceparent.java

+    /**
+     * 16 byte trace-id that is used to uniquely identify a distributed trace
+     */
+    @VisibleForTesting final String traceId;


Since you have public accessors, you shouldn't need @VisibleForTesting. Same for spanId and traceFlags

littleaj · 2018-11-28T20:24:03Z

...ava/com/microsoft/applicationinsights/web/internal/correlation/tracecontext/Traceparent.java

+     * @return boolean
+     */
+    private static boolean isHex(String s, int n) {
+        if (s == null || s.length() == 0) {


One option is to Long.parseLong(s, 16) and if it throws NumberFormatException, return false.

Sometimes try/catch can be more performant, but if that's not a consideration at this point, I wouldn't worry about it. Also, I'd write a test to confirm prove it's more performant if you do choose that method.

Yes, I and Reiley discussed this one. @reyang we should have this in final version. By the way, the catch here is also that only lower case should be there. I am not sure what parseLong will do for upper case.

littleaj · 2018-11-28T20:34:37Z

...ava/com/microsoft/applicationinsights/web/internal/correlation/tracecontext/Traceparent.java

+        if (arr.length != 4) {
+            return null;
+        }
+        if (!isHex(arr[0], 2)) {


Depending on the usage here, you should consider replacing the isHex checks and instead assign the Character.digit calls to local variables, then check for -1 since the .digit method is doing these checks already.

littleaj · 2018-11-28T20:35:14Z

...java/com/microsoft/applicationinsights/web/internal/correlation/tracecontext/Tracestate.java

+ *
+ * Implementations can add vendor specific details here.
+ */
+@Experimental


Is something looking for these annotations?

No I have marked this just because, ultimately this classes would be part of tracecontext libraray under different package name. Therefore, I just want to ensure no one depends on it.

littleaj · 2018-11-28T20:37:02Z

...ava/com/microsoft/applicationinsights/web/internal/correlation/tracecontext/Traceparent.java

+    /**
+     * An 8-bit field that controls tracing flags such as sampling, trace level etc.
+     */
+    @VisibleForTesting


Perhaps @VisibleForTesting isn't necessary since there are public accessors.

...ava/com/microsoft/applicationinsights/web/internal/correlation/tracecontext/Traceparent.java

dhaval24 · 2018-11-28T21:57:24Z

@littleaj as this is classes are not perhaps the final version, and there will be some refinements, merging this branch into feature branch and we shall iterate on it.

littleaj · 2018-11-28T22:14:53Z

@dhaval24 sounds good

lmolkova · 2018-11-29T00:09:53Z

...ava/com/microsoft/applicationinsights/web/internal/correlation/tracecontext/Traceparent.java

+     * The constructor that tries to create Traceparent Object from given version, traceId, spanID
+     * and traceFlags.
+     */
+    public Traceparent(int version, String traceId, String spanId, int traceFlags) {


int traceFlags why int? it's byte

lmolkova · 2018-11-29T00:18:29Z

...ava/com/microsoft/applicationinsights/web/internal/correlation/tracecontext/Traceparent.java

+            return null;
+        }
+
+        return new Traceparent(


so in some cases, when traceparent is not valid we return null and in some cases we throw. this is inconsistent. I'd prefer we never throw.
The problem is that if you've got bad traceparent - it's not even your fault and you cannot do anything about it.

Of course you can ask everyone who creates traceparent to catch exceptions and check for nulls, but it's easier to do this once in traceparent

And if it's not valid return a new one - with brand new values - that's what spec also states

lmolkova · 2018-11-29T00:22:57Z

...java/com/microsoft/applicationinsights/web/internal/correlation/tracecontext/Tracestate.java

+    /**
+     * Field to capture tracestate header
+     */
+    public final String reserved;


shouldn't it be private?

I think so, this should be private. @reyang ?

reyang · 2018-11-29T00:50:56Z

This will be removed and replace by the actual implementation :)

* W3C : Tracecontext classes (#775) * W3C Tracecontext * adding comments * addressing PR comments * Implement W3C tracestate * address review feedbacks * address review comments * W3C Integration (#782) * Initial integration of W3C protocol for incoming request * refactoring header name * updating the verbosity level when correlation fails * Fix bugs in correlation found via tests, implement more unit tests and address partial PR comments * improve createOutboundTracestate() * create tracestate when header not available * fix test * enable outbout w3c * Refactering code to use Helper methods with TraceContext classes * fix tracestate integration, fix outbound tracestate injection, fix tests, propogate traceflags * add property to turn on W3C in springboot starter, remove debug logs * adopt internal storage of id's to legacy AI format for backport, update tests * address PR comments * Fix an incorrect assert * refactor resolveCorrelation() method to be more readable and debuggable * rename method names, create outbound traceparent for http if there is no incoming request too * fixing a bug in w3c config for agent * fix the dependency type name, fix target to be host+port | target * update changelog and readme * Minor changes, add log trace to check W3C enabled on agent * adding support for backport with AI-Legacy-Correlation-Headers * adding tests for backward compatibility * address PR comments * add few tests, comments * add backport switch turn off test * reset after tests, to fix the build * adding tracing to understand when W3C is turned on for debugging * fix format string * fix a bug in creating correct target for dependencies

dhaval24 added 2 commits November 28, 2018 11:03

W3C Tracecontext

e8c4e09

adding comments

5cf2bda

dhaval24 added the Enhancement label Nov 28, 2018

dhaval24 self-assigned this Nov 28, 2018

dhaval24 requested a review from littleaj November 28, 2018 19:25

reyang reviewed Nov 28, 2018

View reviewed changes

...ava/com/microsoft/applicationinsights/web/internal/correlation/tracecontext/Traceparent.java Outdated Show resolved Hide resolved

reyang reviewed Nov 28, 2018

View reviewed changes

...java/com/microsoft/applicationinsights/web/internal/correlation/tracecontext/Tracestate.java Outdated Show resolved Hide resolved

reyang reviewed Nov 28, 2018

View reviewed changes

reyang suggested changes Nov 28, 2018

View reviewed changes

...ava/com/microsoft/applicationinsights/web/internal/correlation/tracecontext/Traceparent.java Outdated Show resolved Hide resolved

addressing PR comments

3aee8a0

reyang approved these changes Nov 28, 2018

View reviewed changes

reyang suggested changes Nov 28, 2018

View reviewed changes

littleaj reviewed Nov 28, 2018

View reviewed changes

dhaval24 changed the base branch from master to W3CImplementation November 28, 2018 21:23

dhaval24 merged commit 76b2d92 into W3CImplementation Nov 28, 2018

lmolkova reviewed Nov 29, 2018

View reviewed changes

dhaval24 mentioned this pull request Nov 29, 2018

Initial integration of W3C protocol for incoming request #776

Closed

3 tasks

This was referenced Dec 7, 2018

W3C Integration #782

Merged

W3C Final implementation #785

Merged

trask deleted the W3C/Implementation-Part-1 branch September 21, 2020 00:17

W3C : Tracecontext classes #775

W3C : Tracecontext classes #775

Uh oh!

Conversation

dhaval24 commented Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

reyang left a comment

Choose a reason for hiding this comment

Uh oh!

reyang commented Nov 28, 2018

Uh oh!

reyang left a comment

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!

dhaval24 Nov 28, 2018 • 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!

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!

dhaval24 commented Nov 28, 2018

Uh oh!

littleaj commented Nov 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmolkova Nov 29, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reyang commented Nov 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dhaval24 commented Nov 28, 2018 •

edited

Loading

dhaval24 Nov 28, 2018 •

edited

Loading

lmolkova Nov 29, 2018 •

edited

Loading