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

adding AWS SQS tests to Apache Camel instrumentation #2061

Merged
merged 5 commits into from Jan 20, 2021
Merged

adding AWS SQS tests to Apache Camel instrumentation #2061

merged 5 commits into from Jan 20, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jan 15, 2021

Closes #2060

Adding SQS tests for Apache Camel.
For this purpose, additional library (SQS local mock in fact) is added - https://github.com/softwaremill/elasticmq, licensed under Apache 2.0 (https://github.com/softwaremill/elasticmq/blob/master/LICENSE.txt).

Comment on lines 138 to 141
trace(3, 1) {
it.span(0) {
name "SQS.ReceiveMessage"
kind CLIENT
Copy link
Member

Choose a reason for hiding this comment

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

Just a question: can you explain those separate AWS CLIENT traces? I'm interested in a reason why those spans appear separately

Copy link
Author

Choose a reason for hiding this comment

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

Delete is easy - it's just "cleanup" done after processing batch, so after produce / consume trace is done.

Copy link
Author

Choose a reason for hiding this comment

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

"Receive" spans are quite a different story - it took me while to understand whole process :D

Basically, when SQS consumer (bean) is up, it starts sending "Receive" messages to SQS. Each such message can be seen as a CONSUMER, but only after a message is in fact available and consumed. So, once one of the messages succeeds, Camel starts "consumer" route and that's where Camel's instrumentation kicks in, creating a CONSUMER span. As there is a propagation enabled for Camel, the CONSUMER is tied to Camel's PRODUCER span issued earlier. So far so good. AWS SDK instrumentation does not have a trace propagation (!), therefore "RECEIVE" span will hang detached from the internal-producer-consumer trace.

Now things are getting interesting. Once I enable trace propagation for AWS SDK (and change span KIND for SQS "ReceiveMessage" which IMO should be CONSUMER not CLIENT), there are two options:

  • two CONSUMER spans in the trace: internal - producer (camel) - consumer (AWS SDK) - consumer (camel)
  • camel consumer will be suppressed (!) and the trace will have one consumer span - the one from AWS SDK: internal - producer (camel) - consumer (AWS SDK)

@anuraaga @trask @jkwatson I'd very much like to hear your opinions on the above question (what should happen when there is Camel and AWS SDK with trace propagation) ?

By the way - thanks @mateuszrzeszutek for bringing up this matter :)

Copy link
Member

Choose a reason for hiding this comment

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

I think it'd make the most sense if camel spans were internal - in the context of this test:

INTERNAL input (camel)
--> INTERNAL sqsCamelTest (camel)
    --> PRODUCER SQS.SendMessage (AWS SDK)
        --> CONSUMER SQS.ReceiveMessage (AWS SDK)
            --> INTERNAL sqsCamelTest (camel)

Camel is just a wrapper library that does not send any message to any queue directly, it relies on other libraries - and it makes sense for the camel instrumentation to rely on other instrumentations (AWS SDK here).

We need to have trace propagation in AWS SDK and change SQS spans to PRODUCER/CONSUMER for it to happen though.

kuba-wu and others added 2 commits January 15, 2021 18:52
Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
kuba-wu and others added 2 commits January 18, 2021 11:00
Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
@ghost ghost requested review from mateuszrzeszutek and anuraaga January 18, 2021 12:41
@ghost
Copy link
Author

ghost commented Jan 18, 2021

Switched to testcontainers / localstack. Pls have another look at this.

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks!

@anuraaga anuraaga merged commit 422e1eb into open-telemetry:master Jan 20, 2021
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.

SQS tests for Apache Camel instrumentation
3 participants