-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Refactoring tracing interfaces to camel-api
and camel-support
#15163
base: main
Are you sure you want to change the base?
Conversation
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🤖 CI automation will test this PR automatically. 🐫 Apache Camel Committers, please review the following items:
|
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 not entirely sure I understand the motivations for the change, so marking as requested changes to wait for more feedback.
@@ -67,6 +67,7 @@ | |||
@ConstantProvider("org.apache.camel.ExchangeConstantProvider") | |||
public interface Exchange extends VariableAware { | |||
|
|||
String ACTIVE_SPAN = "OpenTracing.activeSpan"; |
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.
qq: this is not following the pattern we use. Is there a reason for that?
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.
This was copied from camel-trace
, but I agree that it should be renamed.
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.
This key is also used in core/camel-support/src/main/java/org/apache/camel/saga/InMemorySagaCoordinator.java
The idea of this pull request is to move functionality from |
2140f2a
to
8f9a1bb
Compare
This requires a JIRA for tracking something like this happening so users can see this in changelogs |
The decorates can be moved to each component - however so far it has not been needed and having them in one place allows to easier identify which decorators we have. Also have you had a need for a decorator to rely directly on its component APIs vs as it is now. |
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 not sure what would be the advantages of having decorators directly in the components.. If we place all of them in a single point, we could easily know what we support and what we don't, even for catalog purpose. If we move them in the single components it would be much more complicated.
This allows moving the span decorators closer to the components.
8f9a1bb
to
7804e67
Compare
This allows moving the span decorators closer to the components.
Description
Move tracing span interfaces to
camel-api
and abstract implementations tocamel-support
. Refactoring the current span property as aExchangePropertyKey
/internal property.Target
camel-3.x
, whereas Camel 4 uses themain
branch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTests
locally from root folder and I have committed all auto-generated changes.