-
Notifications
You must be signed in to change notification settings - Fork 1k
Allow custom temporary NATS subjects detection #15736
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
5f843ac
bf1f4f9
c661b22
7945c04
281c5a3
c94df32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,15 +10,23 @@ | |
| import com.google.errorprone.annotations.CanIgnoreReturnValue; | ||
| import io.opentelemetry.api.OpenTelemetry; | ||
| import io.opentelemetry.instrumentation.nats.v2_17.internal.NatsInstrumenterFactory; | ||
| import io.opentelemetry.instrumentation.nats.v2_17.internal.NatsSubjectPattern; | ||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.Collection; | ||
| import java.util.List; | ||
| import java.util.regex.Pattern; | ||
|
|
||
| /** A builder of {@link NatsTelemetry}. */ | ||
| public final class NatsTelemetryBuilder { | ||
|
|
||
| private final OpenTelemetry openTelemetry; | ||
|
|
||
| private List<String> capturedHeaders = emptyList(); | ||
| private List<Pattern> temporaryPatterns = DEFAULT_TEMPORARY_PATTERNS; | ||
|
|
||
| public static final List<Pattern> DEFAULT_TEMPORARY_PATTERNS = | ||
| Arrays.asList(NatsSubjectPattern.compile("_INBOX.*"), NatsSubjectPattern.compile("_R_.*")); | ||
|
|
||
| NatsTelemetryBuilder(OpenTelemetry openTelemetry) { | ||
| this.openTelemetry = openTelemetry; | ||
|
|
@@ -35,10 +43,37 @@ public NatsTelemetryBuilder setCapturedHeaders(Collection<String> capturedHeader | |
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Configures the patterns used for temporary subjects. Eg: `^_INBOX\\..+$` | ||
| * | ||
| * @param temporaryPatterns A list of patterns. | ||
| */ | ||
| @CanIgnoreReturnValue | ||
| public NatsTelemetryBuilder setTemporaryPatterns(Collection<Pattern> temporaryPatterns) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps it would be better to let users specify prefixes for temporary subjects? Or do you believe there are use cases where the more powerful matching capability of regex is needed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know all usages but it might not always be prefixes. I guess one could use |
||
| this.temporaryPatterns = new ArrayList<>(temporaryPatterns); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Configures the subjects used for temporary subjects. Eg: `_INBOX.*` | ||
| * | ||
| * @param temporarySubjects A list of subjects. | ||
| */ | ||
| @CanIgnoreReturnValue | ||
| public NatsTelemetryBuilder setTemporarySubjects(Collection<String> temporarySubjects) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Imo in its current for too much effort is needed to understand what this method does and how it differs from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted a nats-friendly API where you specify the temp subjects the same way you'd do them in NATS. But This can be removed of course
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be best to document stuff like that. You should not expect people reading the code really know what kind of wildcards nats uses and to me it feels very unlikely that anybody looking at this method can figure out that it accepts that kind of wildcards. |
||
| this.temporaryPatterns = new ArrayList<>(); | ||
| for (String subject : temporarySubjects) { | ||
| this.temporaryPatterns.add(NatsSubjectPattern.compile(subject)); | ||
| } | ||
| return this; | ||
| } | ||
|
|
||
| /** Returns a new {@link NatsTelemetry} with the settings of this {@link NatsTelemetryBuilder}. */ | ||
| public NatsTelemetry build() { | ||
| return new NatsTelemetry( | ||
| NatsInstrumenterFactory.createProducerInstrumenter(openTelemetry, capturedHeaders), | ||
| NatsInstrumenterFactory.createConsumerProcessInstrumenter(openTelemetry, capturedHeaders)); | ||
| NatsInstrumenterFactory.createProducerInstrumenter( | ||
| openTelemetry, capturedHeaders, temporaryPatterns), | ||
| NatsInstrumenterFactory.createConsumerProcessInstrumenter( | ||
| openTelemetry, capturedHeaders, temporaryPatterns)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,11 +9,17 @@ | |
| import io.opentelemetry.instrumentation.api.incubator.semconv.messaging.MessagingAttributesGetter; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.regex.Pattern; | ||
| import javax.annotation.Nullable; | ||
|
|
||
| enum NatsRequestMessagingAttributesGetter | ||
| class NatsRequestMessagingAttributesGetter | ||
| implements MessagingAttributesGetter<NatsRequest, Object> { | ||
| INSTANCE; | ||
|
|
||
| private final List<Pattern> temporaryPatterns; | ||
|
|
||
| public NatsRequestMessagingAttributesGetter(List<Pattern> temporaryPatterns) { | ||
| this.temporaryPatterns = temporaryPatterns; | ||
| } | ||
|
|
||
| @Override | ||
| public String getSystem(NatsRequest request) { | ||
|
|
@@ -29,15 +35,13 @@ public String getDestination(NatsRequest request) { | |
| @Nullable | ||
| @Override | ||
| public String getDestinationTemplate(NatsRequest request) { | ||
| if (isTemporaryDestination(request)) { | ||
| return request.getInboxPrefix(); | ||
| } | ||
| return null; | ||
| Pattern pattern = getTemporaryPattern(request); | ||
| return pattern == null ? null : pattern.pattern(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isTemporaryDestination(NatsRequest request) { | ||
| return request.getSubject().startsWith(request.getInboxPrefix()); | ||
| return getTemporaryPattern(request) != null; | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -88,4 +92,21 @@ public List<String> getMessageHeader(NatsRequest request, String name) { | |
| List<String> result = headers.get(name); | ||
| return result == null ? Collections.emptyList() : result; | ||
| } | ||
|
|
||
| /** | ||
| * @return the temporary pattern used for this request, or null | ||
| */ | ||
| private Pattern getTemporaryPattern(NatsRequest request) { | ||
| if (request.getSubject().startsWith(request.getInboxPrefix())) { | ||
AlixBa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return NatsSubjectPattern.compile(request.getInboxPrefix() + "*"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. feels inefficient
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perfomance wise?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, I suspect you could avoid compiling the regular expression |
||
| } | ||
|
|
||
| for (Pattern pattern : temporaryPatterns) { | ||
| if (pattern.matcher(request.getSubject()).matches()) { | ||
| return pattern; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.instrumentation.nats.v2_17.internal; | ||
|
|
||
| import java.util.regex.Pattern; | ||
|
|
||
| /** | ||
| * This class is internal and is hence not for public use. Its APIs are unstable and can change at | ||
| * any time. Exposed for {@link io.nats.client.impl.OpenTelemetryDispatcherFactory}. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bad copy/paste 😬 |
||
| */ | ||
| public class NatsSubjectPattern { | ||
|
|
||
| private NatsSubjectPattern() {} | ||
|
|
||
| public static Pattern compile(String subject) { | ||
| return Pattern.compile( | ||
| "^" + subject.replace(".", "\\.").replace(">", "*").replace("*", ".*") + "$"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. none of the tests fail when |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do I understand correctly that this isn't really needed for the javaagent instrumentation to function since this class is only used by the library instrumentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but Muzzle complained because the package of the
OpenTelemetryDispatcherFactoryis set toio.nats.client.impl.to access package-private classes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just curios why it wasn't needed previously. Can't spot what in the current PR requires adding it, but otherwise I'm fine with adding it.