Skip to content
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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ammachado
Copy link
Contributor

This allows moving the span decorators closer to the components.

Description

Move tracing span interfaces to camel-api and abstract implementations to camel-support. Refactoring the current span property as a ExchangePropertyKey/internal property.

Target

  • I checked that the commit is targeting the correct branch (note that Camel 3 uses camel-3.x, whereas Camel 4 uses the main branch)

Tracking

  • If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it).

Apache Camel coding standards and style

  • I checked that each commit in the pull request has a meaningful subject line and body.
  • I have run mvn clean install -DskipTests locally from root folder and I have committed all auto-generated changes.

Copy link
Contributor

🌟 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:

  • First-time contributors require MANUAL approval for the GitHub Actions to run

  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot.

  • You can label PRs using build-all, build-dependents, skip-tests and test-dependents to fine-tune the checks executed by this PR.

  • Build and test logs are available in the Summary page. Only Apache Camel committers have access to the summary.

  • ⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

Copy link
Contributor

@orpiske orpiske left a 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";
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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

@ammachado
Copy link
Contributor Author

I'm not entirely sure I understand the motivations for the change, so marking as requested changes to wait for more feedback.

The idea of this pull request is to move functionality from camel-trace to the core libraries, and move all span decorators to their respective components.

@ammachado ammachado force-pushed the trace-improvements branch 6 times, most recently from 2140f2a to 8f9a1bb Compare August 16, 2024 15:35
@ammachado ammachado marked this pull request as draft August 19, 2024 01:32
@davsclaus
Copy link
Contributor

This requires a JIRA for tracking something like this happening so users can see this in changelogs

@davsclaus
Copy link
Contributor

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.

Copy link
Contributor

@oscerd oscerd left a 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants