Links API proposal #204

marcingrzejszczak · 2023-03-16T15:11:38Z

sonatype-lift · 2023-03-16T15:15:51Z

micrometer-tracing/src/main/java/io/micrometer/tracing/exporter/FinishedSpan.java

@@ -160,4 +162,33 @@ default String getLocalIp() {
     */
    FinishedSpan setRemoteServiceName(String remoteServiceName);

+    /**
+     * @return links


MissingSummary: A summary line is required on public/protected Javadocs.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-03-16T15:15:54Z

...-tracing-bridge-brave/src/main/java/io/micrometer/tracing/brave/bridge/BraveSpanBuilder.java

+    public Span.Builder addLink(TraceContext traceContext) {
+        brave.propagation.TraceContext braveContext = BraveTraceContext.toBrave(traceContext);
+        long nextId = LinkUtils.nextIndex(this.tags);
+        this.tags.put(LinkUtils.spanIdKey(nextId), braveContext.spanIdString());


NULL_DEREFERENCE: object braveContext last assigned on line 155 could be null and is dereferenced at line 157.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-03-16T15:15:57Z

...rometer-tracing-bridge-brave/src/main/java/io/micrometer/tracing/brave/bridge/LinkUtils.java

+
+    private static final String LINKS_PREFIX = "links[";
+
+    private static final String TRACE_ID = "links[%s].traceId";


InlineFormatString: Prefer to create format strings inline, instead of extracting them to a single-use constant

❗❗ 2 similar findings have been found in this PR

🔎 Expand here to view all instances of this finding

File Path Line Number

micrometer-tracing-bridges/micrometer-tracing-bridge-brave/src/main/java/io/micrometer/tracing/brave/bridge/LinkUtils.java 38

micrometer-tracing-bridges/micrometer-tracing-bridge-brave/src/main/java/io/micrometer/tracing/brave/bridge/LinkUtils.java 36

Visit the Lift Web Console to find more details in your report.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-03-16T15:16:00Z

🛠 Lift Auto-fix

Some of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.¹

# Download the patch
curl https://lift.sonatype.com/api/patch/github.com/micrometer-metrics/tracing/204.diff -o lift-autofixes.diff

# Apply the patch with git
git apply lift-autofixes.diff

# Review the changes
git diff

Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command:

curl https://lift.sonatype.com/api/patch/github.com/micrometer-metrics/tracing/204.diff | git apply

Once you're satisfied, commit and push your changes in your project.

You can preview the patch by opening the patch URL in the browser. ↩

jonatan-ivanov · 2023-03-16T21:06:04Z

micrometer-tracing/src/main/java/io/micrometer/tracing/Span.java

+         * @return this
+         * @since 1.1.0
+         */
+        default Builder addLink(TraceContext traceContext) {


If links are used to link Spans, should not this receive a Span instead of the context? Internally it can fetch the context from the Span but the user could do:

span.addLink(otherSpan);

instead of

span.addLink(otherSpan.context());

WDYT?

jonatan-ivanov · 2023-03-16T21:18:44Z

...ests/micrometer-tracing-test/src/main/java/io/micrometer/tracing/test/simple/SimpleSpan.java

+    }
+
+    @Override
+    public Map<TraceContext, Map<String, String>> getLinks() {


Should not we use a Link interface everywhere in our public APIs and convert Links to Map at the end when we need it?

jonatan-ivanov · 2023-03-16T21:21:30Z

...ests/micrometer-tracing-test/src/main/java/io/micrometer/tracing/test/simple/SimpleLink.java

+     * Creates a new instance of {@link SimpleLink}.
+     * @param traceContext trace context
+     */
+    public SimpleLink(TraceContext traceContext) {


Should not we use Span in the parameter instead?

- created Link class instead of working with maps of contexts and tags - Link now can accept different parameters (span, trace context, tags) - adopted all methods to work with lists of links instead of maps - test assertions got simplified thanks to this

sonatype-lift · 2023-03-17T12:36:08Z

...-tracing-bridge-brave/src/main/java/io/micrometer/tracing/brave/bridge/BraveSpanBuilder.java

+    public Span.Builder addLink(Link link) {
+        brave.propagation.TraceContext braveContext = BraveTraceContext.toBrave(link.getTraceContext());
+        long nextId = LinkUtils.nextIndex(this.tags);
+        this.tags.put(LinkUtils.spanIdKey(nextId), braveContext.spanIdString());


NULL_DEREFERENCE: object braveContext last assigned on line 156 could be null and is dereferenced at line 158.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

elisabeteaprcd · 2023-03-17T14:13:42Z

Looks good to me. Thank you.

Links API proposal; fixes gh-182

6443fab

marcingrzejszczak added the enhancement New feature or request label Mar 16, 2023

marcingrzejszczak requested review from jonatan-ivanov and shakuzen March 16, 2023 15:11

sonatype-lift bot reviewed Mar 16, 2023

View reviewed changes

marcingrzejszczak mentioned this pull request Mar 16, 2023

Suport for create span with links #182

Closed

jonatan-ivanov reviewed Mar 16, 2023

View reviewed changes

Changes following the review

3d26067

- created Link class instead of working with maps of contexts and tags - Link now can accept different parameters (span, trace context, tags) - adopted all methods to work with lists of links instead of maps - test assertions got simplified thanks to this

sonatype-lift bot reviewed Mar 17, 2023

View reviewed changes

Fixed javadocs

ac47d76

marcingrzejszczak merged commit 3fe98ee into main Mar 21, 2023
2 checks passed

jonatan-ivanov deleted the links branch March 21, 2023 20:43

jonatan-ivanov added this to the 1.1.0-M3 milestone Mar 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Links API proposal #204

Links API proposal #204

marcingrzejszczak commented Mar 16, 2023

sonatype-lift bot Mar 16, 2023

sonatype-lift bot Mar 16, 2023

sonatype-lift bot Mar 16, 2023

sonatype-lift bot commented Mar 16, 2023

jonatan-ivanov Mar 16, 2023

jonatan-ivanov Mar 16, 2023

jonatan-ivanov Mar 16, 2023

sonatype-lift bot Mar 17, 2023

elisabeteaprcd commented Mar 17, 2023

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file


		private static final String LINKS_PREFIX = "links[";

		private static final String TRACE_ID = "links[%s].traceId";

File Path	Line Number
micrometer-tracing-bridges/micrometer-tracing-bridge-brave/src/main/java/io/micrometer/tracing/brave/bridge/LinkUtils.java	38
micrometer-tracing-bridges/micrometer-tracing-bridge-brave/src/main/java/io/micrometer/tracing/brave/bridge/LinkUtils.java	36

Links API proposal #204

Links API proposal #204

Conversation

marcingrzejszczak commented Mar 16, 2023

sonatype-lift bot Mar 16, 2023

Choose a reason for hiding this comment

sonatype-lift bot Mar 16, 2023

Choose a reason for hiding this comment

sonatype-lift bot Mar 16, 2023

Choose a reason for hiding this comment

sonatype-lift bot commented Mar 16, 2023

🛠 Lift Auto-fix

Footnotes

jonatan-ivanov Mar 16, 2023

Choose a reason for hiding this comment

jonatan-ivanov Mar 16, 2023

Choose a reason for hiding this comment

jonatan-ivanov Mar 16, 2023

Choose a reason for hiding this comment

sonatype-lift bot Mar 17, 2023

Choose a reason for hiding this comment

elisabeteaprcd commented Mar 17, 2023