feat: Introduced new APM module to handle RabbitMQ messaging

[ekrucio]

Messaging systems are one of the main components powering distributed microservices. We're tightly dependent on RabbitMQ messaging in our codebase, and especially this package streadway/amqp .

This PR is related to issue Feature #37 but does not resolve it, it is simply a POC.
If you're okay with those changes I will add tests.

[apmmachine]

❕ Build Aborted

The PR is not allowed to run in the CI yet

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-12-03T17:20:21.517+0000

• Duration: 5 min 37 sec

Steps errors

Expand to view the steps failures

Load a resource file from a library

• Took 0 min 0 sec . View more details here
• Description: approval-list/elastic/apm-agent-go.yml

Error signal

• Took 0 min 0 sec . View more details here
• Description: githubApiCall: The REST API call https://api.github.com/orgs/elastic/members/ekrucio return the message : java.lang.Exception: httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/ekrucio : httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/ekrucio : Code: 404Error: {"message":"User does not exist or is not a member of the organization","documentation_url":"https://docs.github.com/rest/reference/orgs#check-organization-membership-for-a-user"}

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• run benchmark tests : Run the benchmark test.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[axw]

@ekrucio thanks for opening the PR!

The Extractor interface feels a bit too specific to the pattern you are using in your application. Because streadway/amqp provides a fairly general API, I think we may have to provide a toolkit of functions for creating transactions or spans for consumers, rather than always assuming a one-to-one mapping between messages and transactions (effectively this pattern, IIANM: https://github.com/elastic/apm/blob/main/specs/agents/tracing-instrumentation-messaging.md#passive-message-reception)

Are you defining a Publisher interface in your code, so you can use either an amqp.Channel or an amqpstreadway.Publisher? If that is the case, I think the Publisher could stay - but it does not need to be an interface; it should instead be a struct type.

[ekrucio]

@ekrucio thanks for opening the PR!

The Extractor interface feels a bit too specific to the pattern you are using in your application. Because streadway/amqp provides a fairly general API, I think we may have to provide a toolkit of functions for creating transactions or spans for consumers, rather than always assuming a one-to-one mapping between messages and transactions (effectively this pattern, IIANM: https://github.com/elastic/apm/blob/main/specs/agents/tracing-instrumentation-messaging.md#passive-message-reception)

Are you defining a Publisher interface in your code, so you can use either an amqp.Channel or an amqpstreadway.Publisher? If that is the case, I think the Publisher could stay - but it does not need to be an interface; it should instead be a struct type.

My idea about the Publisher interface was to be some kind of a wrapper over the ch.Publish and thus the way to get access to this "custom publisher" we should call Wrap(ch *amqp.Channel).
// Wrap wraps amqp.Channel such that its Publish method calls streadway/amqp.Publish.
I do undertstand its somewhat constraining, but streadway/amqp is really extensive in what you can do with a streadway/amqp.Channel . Do we aim for this module to be a complete wrapper over them, or we should go "piece by piece" and the community could fill the needed, especially since the precedent/example would be available?

I completely agree with the unneeded and wrong "contract" initiated by the current implementation of the Extractor. Should the change to return trace context be suffiecient to fix this?

[axw]

My idea about the Publisher interface was to be some kind of a wrapper over the ch.Publish and thus the way to get access to this "custom publisher" we should call Wrap(ch *amqp.Channel).

👍

I do undertstand its somewhat constraining, but streadway/amqp is really extensive in what you can do with a streadway/amqp.Channel . Do we aim for this module to be a complete wrapper over them, or we should go "piece by piece" and the community could fill the needed, especially since the precedent/example would be available?

I think starting out minimally is fine, and it can be filled in later.

For Publisher, I'm mostly just bothered about exporting an interface rather than a concrete struct type. What do you think about having a Channel wrapper struct type that embeds amqp.Channel, and overwrites its Publish method? Something like:

// WrappedChannel wraps amqp.Channel such that Publish calls are traced,
// and trace context is added to published messages.
//
// Trace context must be supplied using Channel.WithContext.
type WrappedChannel struct {
    *amqp.Channel
    ctx context.Context
}

func WrapChannel(ch amqp.Channel) WrappedChannel {
    ...
}

func (c WrappedChannel) WithContext(ctx context.Context) WrappedChannel {
    return WrappedChannel{Channel: c.Channel, ctx: ctx}
}

// Publish publishes a message and returns an error in encountered.
func (c WrappedChannel) Publish(exchange, key string, mandatory, immediate bool, msg amqp.Publishing) error {
    ...
}

I completely agree with the unneeded and wrong "contract" initiated by the current implementation of the Extractor. Should the change to return trace context be suffiecient to fix this?

👍 Changing Extractor to some kind of ExtractTraceContext function would resolve that part.

[ekrucio]

For Publisher, I'm mostly just bothered about exporting an interface rather than a concrete struct type. What do you think about having a Channel wrapper struct type that embeds amqp.Channel, and overwrites its Publish method? Something like:

// WrappedChannel wraps amqp.Channel such that Publish calls are traced,
// and trace context is added to published messages.
// ...

That is better, I agree, will do it that way 👍 .

[ekrucio]

@axw Just fyi I made the necessary changes (or at least I think so).

[axw]

@ekrucio thanks for the changes, this looks like it's in a good shape now. I've left a few more comments, otherwise it needs some tests, copyright headers, and doc comments.

[ekrucio]

@axw Sorry, I didn't read you whole comment and requested a review before resolving everything mentioned here ->

I've left a few more comments, otherwise it needs some tests, copyright headers, and doc comments.

Will add those 👍

[ekrucio]

@axw Added tests but due to the conctrete implementation of this library the Publish test is less than pretty.
In a nutshell, we have no way to mock the ch.Publish method without a lot of unnecessary boilerplate code.
Issues showcasing this ->
streadway/amqp/issues/164
streadway/amqp/issues/471
streadway/amqp/issues/122
streadway/amqp/issues/64
My understanding is that it is not our concern how the publish action went throught, and we want to test the context propagation, that's why I decided to recover from the panic and assert the headers only.
I would understand if you're not okay with that, but I see no other way, either this panic recovery or trying to abstract the whole communication.

[ekrucio]

@axw I've added the proposed fixes. Also your idea about the in-memory AMQP server was top! I was ashamed of the way I did the test initially and it's much better that way. 👍

[axw]

Thanks @ekrucio, the main code all looks good, and the new test is nice :)

I have some concerns about the test being potentially flaky, for which I've left a suggested fix.

Other than that, there's just a few other things that need to be updated:

• please run make scripts/Dockerfile-testing update-modules update-licenses fmt (you can verify the results by running make precheck -- this is required to pass by CI)
• please add a small entry to CHANGELOG.asciidoc, and to docs/instrumenting.asciidoc. See https://github.com/elastic/apm-agent-go/pull/1301/files for an example of where a new instrumentation module was added.

[ekrucio]

* please run `make scripts/Dockerfile-testing update-modules update-licenses fmt` (you can verify the results by running `make precheck` -- this is required to pass by CI)

@axw
Well this didn't go as planned. I'm not sure what Go version I should use, didn't find it mentioned anywhere and I think I have to use 1.15 to successfully complete those steps in the Makefile.
That's the problem, I'm using a MacBook M1 and there's no Go 1.15 for ARM processors.
https://go.dev/dl/
I may be completely mistaken and I could just be doing something wrong. Can you assist me with those steps, or give me pointers if there's something else I'm missing.

[axw]

I may be completely mistaken and I could just be doing something wrong. Can you assist me with those steps, or give me pointers if there's something else I'm missing.

You can use a newer version of Go than 1.15, that's just the minimum for using the agent. I can take a look at fixing these things up and push to your branch.

Would you please add the docs/instrumenting.asciidoc section?

[axw]

You can use a newer version of Go than 1.15, that's just the minimum for using the agent. I can take a look at fixing these things up and push to your branch.

I can't push to your branch. If you're comfortable doing so, please enable that at https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests

[cla-checker-service]

💚 CLA has been signed

[ekrucio]

@axw We had to raise the root Go version to Go 1.17 because one of the packages -> "golang.org/x/sys/unix" requires 1.17, otherwise it doesn't want to compile.
# [golang.org/x/sys/unix](http://golang.org/x/sys/unix) ../../../pkg/mod/golang.org/x/sys@v0.2.0/unix/syscall.go:83:16: undefined: unsafe.Slice ../../../pkg/mod/golang.org/x/sys@v0.2.0/unix/syscall_darwin.go:95:8: undefined: unsafe.Slice ../../../pkg/mod/golang.org/x/sys@v0.2.0/unix/syscall_unix.go:118:7: undefined: unsafe.Slice ../../../pkg/mod/golang.org/x/sys@v0.2.0/unix/sysvshm_unix.go:33:7: undefined: unsafe.Slice note: module requires Go 1.17

[axw]

@axw We had to raise the root Go version to Go 1.17 because one of the packages -> "golang.org/x/sys/unix" requires 1.17, otherwise it doesn't want to compile.

Raising the minimum version for all modules is a showstopper. We support Go 1.15+, and changing that will require a major version bump.

Tests have been passing across the repo with Go 1.15. I don't see any changes to golang.org/x/sys/unix in the PR. Maybe you saw this error while updating module dependencies? Can you please remove the go 1.17 change, and then let me know which specific command fails?

Also, while running the tests, I noticed some race conditions and other errors originating from garagemq and its dependencies. If you run go test -race in apmstreadwayamqp, you should see the same. I think it may be best to revert to your original approach for now.

[Jamess-Lucass]

Sorry if this is a moot point but in the issue it does mention support for streadway/amqp. However this is now a unmaintained fork and in the README states.

"This repository is NOT ACTIVELY MAINTAINED. Consider using a different fork instead: rabbitmq/amqp091-go."

I am not sure if this changes any implementation within this PR but would it be best to move forward with supporting rabbitmq/amqp091-go instead.

I am quite busy with work at the moment but if this pull request is stale I may have a go at implementing a module for RabbitMQ over the coming months.

[ekrucio]

Sorry if this is a moot point but in the issue it does mention support for streadway/amqp. However this is now a unmaintained fork and in the README states.

"This repository is NOT ACTIVELY MAINTAINED. Consider using a different fork instead: rabbitmq/amqp091-go."

I am not sure if this changes any implementation within this PR but would it be best to move forward with supporting rabbitmq/amqp091-go instead.

I am quite busy with work at the moment but if this pull request is stale I may have a go at implementing a module for RabbitMQ over the coming months.

I'm sorry, I've been pretty busy lately and the last I remember is I think the PR is ready to be merged with the exception of changes related to the Go modules which I didn't had to look at.
About the status of the amqp module which is instrumented, I'm not aware of the compatability of them both. My contribution is related to this RabbitMQ library only.
@axw Hi and sorry for the 5 months gap. Since this repository stems from our organization fork, and I would not be able to grant you access, Are you okay with closing this PR and reopening it from my personal account and you could push the needed changes, as you proposed earlier.

[axw]

@axw Hi and sorry for the 5 months gap. Since this repository stems from our organization fork, and I would not be able to grant you access, Are you okay with closing this PR and reopening it from my personal account and you could push the needed changes, as you proposed earlier.

Sure, that's fine - go ahead and close/reopen when you're ready. It may take a while before I get to it though.