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

Updated for Context Propagation and Baggage Data Insert/Extract #58

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

itsJamilAhmed
Copy link

  1. Amended TopicPublisher to publish in persistent mode, add example span attributes and baggage for contextual information.
  2. Amended QueuePublisher to publish in persistent mode, add example span attributes and baggage for contextual information.
  3. Amended QueueSubscriber to create a receive and processing spans. Also now takes queue name as program argument.
  4. Included an example of using HTTP OTLP Endpoints with Authorization headers in TracingUtil.java
  5. Amended pom.xml for version 1.1.0 of solace-opentelemetry-jms-integration
  6. Additional comments and references for OTEL explainers and best practices.

@Mrc0113 Mrc0113 requested a review from TamimiGitHub May 22, 2024 18:08
@Mrc0113
Copy link
Member

Mrc0113 commented May 22, 2024

Thank you for your contribution @itsJamilAhmed. We'll take a look and get it merged soon!

@@ -51,11 +54,12 @@
*/
public class TopicPublisher {

final String TOPIC_NAME = "solace/samples/jms/direct/pub/tracing";
private static final String SERVICE_NAME = "SolaceJMSTopicPublisherManualInstrument";
private static final String SERVICE_NAME = "ACME Product Master [DEV]";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're updating the publishing topic can we also please update the TopicSubscriber.java to reflect those topics

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TopicSubscriber.java was not covered in my changes since a direct subscriber generates no span from the broker. i.e. It is superfluous here as a DT example and should be removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - then I would recommend removing the TopicSubscriber.java and TopicSyncReceiver.java as well from the manualinstrumentation dir

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Samples deleted and build.gradle updated for the generated start scripts.

Copy link
Member

@Mrc0113 Mrc0113 May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@itsJamilAhmed @TamimiGitHub TopicSubscriber.java and TopicSyncReceiver.java might not create a span from the broker (no acknowledgement w/ Direct), but they do show how to create a child span in the client side app. I do feel like that provides value...just b/c the broker doesn't create a span doesn't mean the app itself shouldn't:
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree with renaming the file names for clarity and not have the same file names as found in the patterns dir.

And @itsJamilAhmed the build.gradle file was not committed

Copy link
Contributor

@TamimiGitHub TamimiGitHub May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also realized we will need to remove HowToImplementTracingManualInstrumentation.java

Copy link
Author

@itsJamilAhmed itsJamilAhmed May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with doing a rename on the samples to make the DT ones more specific. (The JCSMP samples are clearer in this regard btw.)
Also on having both a TopicPublisher and QueuePublisher, there is some redundancy yes when the focus here is to show the instrumentation, not how to publish a message to either type...

An argument for keeping it would be around convenience of running the samples. Publishing to a topic needs the user to have configured subscriptions on the queue in order for the queue subscriber to work. Publishing direct to the queue and subsequently subscribing from it makes things more 'out of the box' ready. Conversely, publishing to a topic is the best practice so that should take priority?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also realized we will need to remove HowToImplementTracingManualInstrumentation.java

I don't think we should remove that one. I actually prefer that which gets straight to the point vs. these longer apps where people have to look through more code ot understand what is going on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough yeah that could be used for snippet references

@TamimiGitHub
Copy link
Contributor

Ran the changes locally and the traces look great! will give it another code review and see if there are any other comments needed

@aaron-613
Copy link
Contributor

Hi team, I'm taking a look at this PR now after reviewing the JCSMP DT updates. This will include updating the semconv JAR versions and a few other things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants