Exclude uri from otel tracing #43885

mcruzdev · 2024-10-15T17:02:14Z

What

This pull request creates a new BuildItem that is used for drop uri from tracing when using quarkus-opentelemetry extension. The DropApplicationUrisBuildItem is generated from the scanning of the @Traceless annotation.

The @Traceless annotation is used with the jakarta.ws.rs.Path annotation. If annotated at class level all uri below will be ignored for tracing.

If your @Path annotation contains expression like @Path("/cars/{carId}") the extension will ignore all thing after cars using, e.g. cars*.

Why

You can see the issue below, or directly this Stackoverflow discussion.

Fixes #42659

quarkus-bot · 2024-10-15T17:02:18Z

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

description should not be empty, describe your intent or provide links to the issues this PR is fixing (using Fixes #NNNNN) or changelogs

_{This message is automatically generated by a bot.}

quarkus-bot · 2024-10-15T17:02:35Z

/cc @brunobat (opentelemetry), @radcortez (opentelemetry)

mcruzdev · 2024-10-15T17:02:43Z

First draft pull request. Feel free to add any suggestions!

cc: @brunobat

brunobat

I think this is moving in the right direction @mcruzdev.
I think you should add a property where you could add a list of strings with paths that can be ignored.
it you could add a test in quarkus-integration-test-opentelemetry it would be great.

...rc/test/java/io/quarkus/opentelemetry/deployment/OpenTelemetryTracelessWithRootPathTest.java

...test/java/io/quarkus/opentelemetry/deployment/common/traces/TracelessClassLevelResource.java

github-actions · 2024-10-21T16:30:55Z

🙈 The PR is closed and the preview is expired.

brunobat

@mcruzdev can you please update src/main/asciidoc/opentelemetry-tracing.adoc in the section related with sampling and add 2 sections, one for the annotation and another for the configuration?

Can you also swash and rebase the PR?

brunobat

Thanks @mcruzdev
It's a great PR.
I added just some minor comments.

...n/java/io/quarkus/opentelemetry/runtime/AutoConfiguredOpenTelemetrySdkBuilderCustomizer.java

brunobat · 2024-11-11T16:24:10Z

.../opentelemetry/runtime/src/main/java/io/quarkus/opentelemetry/runtime/tracing/Traceless.java

+/**
+ * Identifies that the current path should not be select for tracing.
+ * <p>
+ * Used together with {@code jakarta.ws.rs.Path} annotation.


Can you please add a test where a @Traceless is used outside of a rest endpoint?
I imagine you get an exception, would it be possible to have a nice message in that case?

We do not have an exception, should we add an exception?

If this only makes sense for rest endpoints, yes.

integration-tests/opentelemetry/src/main/resources/application.properties

mcruzdev · 2024-11-13T12:34:40Z

Sorry, for delay @brunobat. I have been very busy lately.

I answered your comments!

mcruzdev · 2024-11-13T12:44:15Z

...y/deployment/src/test/java/io/quarkus/opentelemetry/deployment/common/traces/NoResource.java

+import io.quarkus.opentelemetry.runtime.tracing.Traceless;
+
+@Traceless
+public class NoResource {


@brunobat This one do not throw an exception, because we have the following rule:

if (Objects.isNull(possibleClassAnnotatedWithPath)) { continue; }

yeah, better throw a precise exception because their code will not behave as expected.

brunobat

Excellent PR. Thanks very much @mcruzdev!

quarkus-bot · 2024-11-13T16:02:12Z

Status for workflow `Quarkus Documentation CI`

This is the status report for running Quarkus Documentation CI on commit 88f628d.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

quarkus-bot · 2024-11-13T16:46:41Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit 88f628d.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 integration-tests/opentelemetry-vertx-exporter

✖ io.quarkus.it.opentelemetry.vertx.exporter.grpc.SimpleGrpcNoTLSNoCompressionTest.test - History

Assertion condition defined as a Lambda expression in io.quarkus.it.opentelemetry.vertx.exporter.AbstractExporterTest Expecting size of: [resource_metrics { resource { attributes { - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.it.opentelemetry.vertx.exporter.AbstractExporterTest 
Expecting size of:
  [resource_metrics {
  resource {
    attributes {
      key: "host.name"
      value {

michalvavrik · 2024-11-25T23:32:55Z

...ry/deployment/src/main/java/io/quarkus/opentelemetry/deployment/tracing/TracerProcessor.java

@@ -170,6 +192,77 @@ void dropNames(
        dropStaticResources.produce(new DropStaticResourcesBuildItem(resources));
    }

+    private Set<String> generateTracelessUris(final List<AnnotationInstance> annotations, final String rootPath) {
+        final Set<String> applicationUris = new HashSet<>();
+        for (AnnotationInstance annotation : annotations) {


Documentation says:

There are two ways to disable tracing for a specific REST endpoint.

I do not believe you can disable tracing for specific REST endpoints based on paths, because you cannot identify REST resource methods with path. Path to resource method is one-to-many relation, but docs ATM says absolutely nothing about that.

I am worried about this annotation approach:

What will happen if I annotate @GET method with certain path? Won't this will also exclude @PUT and any other HTTP method? Same must be true for different consumed/produced types etc. if I have 10 different endpoints with same path and I put this one one endpoint, is it expected that all the endpoints are not traced?

If I placed this one @Path("/hello/{id}") String method() and I have another method @Path("/hello/more-specific") completelyDifferentMethod() then both endpoints will be excluded even though I only placed this on one method with less specific path?

I can have endpoints only annotated with HTTP method like @GET etc. but this feature depends on @Path annotation, so here I cannot disable endpoint get():

@Path("something") public class SomethingResource { @GET public String get() { return "something"; } @GET @Path("another") public String another() { return "another"; } }

I think this won't work for subresources either, you will get illegal state for valid scenario like:

@Path("/simple") public class SimpleQuarkusRestResource { @Traceless @Path("sub") public SubResource subResource() { return new SubResource(); } } public class SubResource { @Path("123") @GET public String sub() { return "sub"; } @GET @Path("otherSub") public String otherPath() { return "otherSub"; } }

it does not consider inheritance, https://jakarta.ee/specifications/restful-ws/4.0/jakarta-restful-ws-spec-4.0#annotationinheritance, if I have @Path on parent class endpoint or interface method without @Path, I'll get exception message containing Please ensure that the class is properly annotated with @Path annotation..

documentation speaks about endpoints, not paths. REST clients has also @Path, what happens if I put this on the REST client? Or better, if I put this on REST endpoints, this will not exclude traces for the client as well?

I suggest that someone who makes living working on Jakarta REST impl. like @geoand reviews this PR. Maybe it's alright, I wouldn't know.

Path to resource method is one-to-many relation, but docs ATM says absolutely nothing about that

This is absolutely true for JAX-RS / Jakarta REST. When I look at @Traceless, I expect it's matching to behave the same way as @PermissionsAllowed for example - this PR does not seem to do that at all

I believe that @Traceless should be removed completely as it's behavior is wrong. Allowing users to configure the path to not be traced makes sense, but with the way things are now, @Traceless is very misleading and get users into a lot of trouble.

I want to make sure to properly acknowledge @mcruzdev for the work done here and @michalvavrik for raising this issue!

It's amazing to have such a great community that works so effectively on continuously improving Quarkus in so many ways!

Thank you for catching this @michalvavrik. I really appreciate your reviews; I’ve been learning a lot.

We can remove the @Traceless annotation and rely solely on the configuration for now until we have a better implementation through annotations.

@geoand can I send a pull request for removing it? From what I’ve seen, this hasn’t been released yet, correct?

cc: @brunobat

Yes please do, this indeed has not been released yet.

Was on a conference yesterday. I've replied in the new issue.

quarkus-bot bot added the area/tracing label Oct 15, 2024

mcruzdev force-pushed the issue-42659 branch from 82c7cb4 to d282746 Compare October 20, 2024 02:20

mcruzdev marked this pull request as ready for review October 20, 2024 02:21

brunobat reviewed Oct 21, 2024

View reviewed changes

...rc/test/java/io/quarkus/opentelemetry/deployment/OpenTelemetryTracelessWithRootPathTest.java Show resolved Hide resolved

...test/java/io/quarkus/opentelemetry/deployment/common/traces/TracelessClassLevelResource.java Show resolved Hide resolved

This comment has been minimized.

Sign in to view

quarkus-bot bot added the triage/flaky-test label Oct 21, 2024

This comment has been minimized.

Sign in to view

mcruzdev requested a review from brunobat October 23, 2024 00:25

brunobat reviewed Oct 23, 2024

View reviewed changes

mcruzdev force-pushed the issue-42659 branch from 58fbee5 to fa69372 Compare October 23, 2024 17:58

quarkus-bot bot added the area/documentation label Oct 23, 2024

mcruzdev force-pushed the issue-42659 branch from fa69372 to 89c838d Compare October 23, 2024 18:05

mcruzdev requested a review from brunobat October 23, 2024 18:05

mcruzdev force-pushed the issue-42659 branch from be9cdba to 1752c26 Compare October 23, 2024 18:06

This comment has been minimized.

Sign in to view

mcruzdev force-pushed the issue-42659 branch from 1752c26 to e31a5b7 Compare October 29, 2024 14:22

This comment has been minimized.

Sign in to view

brunobat requested changes Nov 11, 2024

View reviewed changes

mcruzdev force-pushed the issue-42659 branch from e31a5b7 to f202f18 Compare November 13, 2024 12:41

mcruzdev commented Nov 13, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

Add annotation and cofiguration for disabling tracing

88f628d

mcruzdev force-pushed the issue-42659 branch from f202f18 to 88f628d Compare November 13, 2024 15:21

brunobat approved these changes Nov 13, 2024

View reviewed changes

brunobat added the release/noteworthy-feature label Nov 13, 2024

brunobat merged commit 309646d into quarkusio:main Nov 14, 2024
32 checks passed

quarkus-bot bot added this to the 3.18 - main milestone Nov 14, 2024

quarkus-bot bot added the kind/enhancement New feature or request label Nov 14, 2024

michalvavrik reviewed Nov 25, 2024

View reviewed changes

This was referenced Nov 26, 2024

Support @Traceless for suppressing traces from application URI #44733

Open

Remove @Traceless annotation #44724

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude uri from otel tracing #43885

Exclude uri from otel tracing #43885

mcruzdev commented Oct 15, 2024 •

edited

Loading

quarkus-bot bot commented Oct 15, 2024

quarkus-bot bot commented Oct 15, 2024

mcruzdev commented Oct 15, 2024 •

edited

Loading

brunobat left a comment

This comment has been minimized.

github-actions bot commented Oct 21, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

brunobat left a comment •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

brunobat left a comment

brunobat Nov 11, 2024

mcruzdev Nov 13, 2024

brunobat Nov 13, 2024

mcruzdev commented Nov 13, 2024 •

edited

Loading

mcruzdev Nov 13, 2024 •

edited

Loading

brunobat Nov 13, 2024

This comment has been minimized.

This comment has been minimized.

brunobat left a comment •

edited

Loading

quarkus-bot bot commented Nov 13, 2024

quarkus-bot bot commented Nov 13, 2024 •

edited by github-actions bot

Loading

michalvavrik Nov 25, 2024

geoand Nov 26, 2024 •

edited

Loading

geoand Nov 26, 2024

geoand Nov 26, 2024

geoand Nov 26, 2024

mcruzdev Nov 26, 2024

mcruzdev Nov 26, 2024

geoand Nov 26, 2024

brunobat Nov 27, 2024

Exclude uri from otel tracing #43885

Exclude uri from otel tracing #43885

Conversation

mcruzdev commented Oct 15, 2024 • edited Loading

What

Why

quarkus-bot bot commented Oct 15, 2024

quarkus-bot bot commented Oct 15, 2024

mcruzdev commented Oct 15, 2024 • edited Loading

brunobat left a comment

Choose a reason for hiding this comment

This comment has been minimized.

github-actions bot commented Oct 21, 2024 • edited Loading

This comment has been minimized.

This comment has been minimized.

brunobat left a comment • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

brunobat 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

mcruzdev commented Nov 13, 2024 • edited Loading

mcruzdev Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

brunobat left a comment • edited Loading

Choose a reason for hiding this comment

quarkus-bot bot commented Nov 13, 2024

Status for workflow Quarkus Documentation CI

quarkus-bot bot commented Nov 13, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 integration-tests/opentelemetry-vertx-exporter

Choose a reason for hiding this comment

geoand Nov 26, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcruzdev commented Oct 15, 2024 •

edited

Loading

mcruzdev commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 21, 2024 •

edited

Loading

brunobat left a comment •

edited

Loading

mcruzdev commented Nov 13, 2024 •

edited

Loading

mcruzdev Nov 13, 2024 •

edited

Loading

brunobat left a comment •

edited

Loading

Status for workflow `Quarkus Documentation CI`

quarkus-bot bot commented Nov 13, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`

geoand Nov 26, 2024 •

edited

Loading