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.

Sign up for GitHub

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

Jump to bottom

Aerospike 4 instrumentation #2061

Merged
merged 14 commits into from Nov 16, 2020
Merged

Aerospike 4 instrumentation #2061

merged 14 commits into from Nov 16, 2020

Conversation

mcculls
Copy link
Contributor

@mcculls mcculls commented Nov 9, 2020

No description provided.

@mcculls mcculls force-pushed the mcculls/aerospike4 branch 8 times, most recently from e828acb to 8126ab4 Compare November 11, 2020 17:55
@mcculls mcculls marked this pull request as ready for review November 12, 2020 10:56
@mcculls mcculls requested a review from a team as a code owner November 12, 2020 10:56
richardstartin
richardstartin reviewed Nov 12, 2020
View reviewed changes
.circleci/config.yml
@@ -129,6 +129,8 @@ jobs:
- image: memcached
# This is used by rabbitmq instrumentation tests
- image: rabbitmq
# This is used by aerospike instrumentation tests
- image: aerospike
Copy link
Member

Choose a reason for hiding this comment

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

👍

richardstartin
richardstartin approved these changes Nov 12, 2020
View reviewed changes
Copy link
Member

@richardstartin richardstartin left a comment

Choose a reason for hiding this comment

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

Looks good

tylerbenson
tylerbenson approved these changes Nov 12, 2020
View reviewed changes
Copy link
Contributor

@tylerbenson tylerbenson left a comment •
edited

Choose a reason for hiding this comment

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

I'm also ok with this once the outstanding review items are addressed.

Also, please update the title and description.

...ospike-4/src/main/java/datadog/trace/instrumentation/aerospike4/AbstractTracingListener.java Outdated Show resolved Hide resolved
...rospike-4/src/main/java/datadog/trace/instrumentation/aerospike4/TracingExecuteListener.java Outdated Show resolved Hide resolved
@mcculls mcculls changed the title Aerospike 4 instrumentation (WIP) Aerospike 4 instrumentation Nov 13, 2020
@mcculls
Don't wrap Runnables belonging to NioEventLoop(s) as they want to pro…
c69d046 
…pagate CloseException

outside of the event loop on close() and wrapping them in FutureTask interferes with that
@mcculls mcculls merged commit d69c337 into master Nov 16, 2020
@mcculls mcculls deleted the mcculls/aerospike4 branch November 16, 2020 12:35
@github-actions github-actions bot added this to the 0.68.0 milestone Nov 16, 2020
@richardstartin richardstartin added the inst: others All other instrumentations label Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tylerbenson tylerbenson tylerbenson approved these changes

@richardstartin richardstartin richardstartin approved these changes

Assignees
No one assigned
Labels
inst: others All other instrumentations
Projects
None yet
Milestone
0.68.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mcculls @tylerbenson @richardstartin