-
Notifications
You must be signed in to change notification settings - Fork 137
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
Drop event bus message spans #3238
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
60 changes: 60 additions & 0 deletions
60
service-base/src/main/java/org/eclipse/hono/service/quarkus/DropBySpanNamePrefixSampler.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
/** | ||
* Copyright (c) 2022 Contributors to the Eclipse Foundation | ||
* | ||
* See the NOTICE file(s) distributed with this work for additional | ||
* information regarding copyright ownership. | ||
* | ||
* This program and the accompanying materials are made available under the | ||
* terms of the Eclipse Public License 2.0 which is available at | ||
* http://www.eclipse.org/legal/epl-2.0 | ||
* | ||
* SPDX-License-Identifier: EPL-2.0 | ||
*/ | ||
package org.eclipse.hono.service.quarkus; | ||
|
||
import java.util.List; | ||
|
||
import io.opentelemetry.api.common.Attributes; | ||
import io.opentelemetry.api.trace.SpanKind; | ||
import io.opentelemetry.context.Context; | ||
import io.opentelemetry.sdk.trace.data.LinkData; | ||
import io.opentelemetry.sdk.trace.samplers.Sampler; | ||
import io.opentelemetry.sdk.trace.samplers.SamplingResult; | ||
|
||
/** | ||
* Sampler that drops spans based on a given list of span name prefixes. | ||
*/ | ||
public class DropBySpanNamePrefixSampler implements Sampler { | ||
|
||
private final Sampler sampler; | ||
private final List<String> spanNamePrefixList; | ||
|
||
/** | ||
* Creates a new DropBySpanNamePrefixSampler. | ||
* | ||
* @param sampler Sampler to use if the span name didn't match the given prefixes. | ||
* @param spanNamePrefixList The list of prefixes to match the span name against for the decision whether to drop | ||
* the span. | ||
*/ | ||
public DropBySpanNamePrefixSampler(final Sampler sampler, final List<String> spanNamePrefixList) { | ||
this.sampler = sampler; | ||
this.spanNamePrefixList = spanNamePrefixList; | ||
} | ||
|
||
@Override | ||
public SamplingResult shouldSample(final Context parentContext, final String traceId, final String spanName, final SpanKind spanKind, | ||
final Attributes attributes, final List<LinkData> parentLinks) { | ||
|
||
for (final String spanNamePrefix : spanNamePrefixList) { | ||
if (spanName.startsWith(spanNamePrefix)) { | ||
return SamplingResult.drop(); | ||
} | ||
} | ||
return sampler.shouldSample(parentContext, traceId, spanName, spanKind, attributes, parentLinks); | ||
} | ||
|
||
@Override | ||
public String getDescription() { | ||
return sampler.getDescription(); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 wonder if we might want to actually use the non-application root path then. Given that we already decided that the 2.0.0 chart will not be backward compatible with Hono 1.x, I think that we could as well take advantage of the existing features then ...
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.
Right, we could do that. (The
DropHttpRequestSpansSampler
would still be needed though for now, as not only the non-application-root-path spans shall get dropped. Issue for the removal of that sampler is #3213.)Changing the non-application-root-path and the healthcheck endpoints should be done in a separate PR then.
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.
Couldn't we simply remove the TracingHandler in favor of the standard functionality provided by the quarkus-opentelemetry extension?
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.
That is basically the goal of #3213 (see also #3205 (comment)).
It's a bit more than just removing the
TracingHandler
class. You mean including this in 2.0.0?I began working on this but then stopped, waiting for the Quarkus update. I can have another look.
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.
Well, if we want to use the non-application-root path for probes and metrics, then I think we should do it with 2.0.0. Not sure how this is related to getting rid of the TracingHandler, but if we can kill two birds with one stone, then I think we should do it.
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.
In any case, this PR here is mostly about the event bus spans. I've only included a small fix for the
DropHttpRequestSpansSampler
regarding the Quarkus update (and the changed span names there).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 take it that we indeed want to change these paths with Hono 2.0.0 so that they include a non-empty non-application-root-path (e.g.
q
), right?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.
With regards to the dropping of the healthcheck endpoint spans, the current Hono code is independent from the used non-application-root-path. As a custom sampler is needed in Hono (for using the
SamplingPrioritySampler
), dropping of these spans needs to be done manually anyway (until a solution for quarkusio/quarkus#25525 is implemented). So, mandating usage of the/q
healthcheck endpoint prefix would only enable us to simplify the span-dropping logic a bit here.In that sense, I have no strong view about switching to use the
/q
prefix. But I guess, as Quarkus is now used for all Hono components and we don't need Helm-chart backwards-compatibility here, we might as well stick to the Quarkus default paths.