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

Add plugin to support Aerospike Java client #565

Merged
merged 6 commits into from
Jun 29, 2023

Conversation

dd1k
Copy link
Contributor

@dd1k dd1k commented Jun 26, 2023

@wu-sheng wu-sheng added this to the 9.0.0 milestone Jun 26, 2023
@wu-sheng
Copy link
Member

Please fix CI, and you should run the build, run UT and test scenarios locally first.

@dd1k
Copy link
Contributor Author

dd1k commented Jun 26, 2023

Running UT and test scenarios locally works fine,pom.xml file forgot to add license header,already fixed。

AbstractSpan span = ContextManager.createExitSpan("Aerospike/" + method.getName(), peer);
span.setComponent(ComponentsDefine.AEROSPIKE);
Tags.CACHE_TYPE.set(span, "Aerospike");
SpanLayer.asCache(span);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't there any command to collect for this plugin? No read/write flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes,It is operated by different methods,example: client.put()、client.get()、client.append()

Copy link
Member

Choose a reason for hiding this comment

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

put and append look like write, get is read.
I think you could add op tag.

import static net.bytebuddy.matcher.ElementMatchers.named;

public enum AerospikeMethodMatch {
INSTANCE;
Copy link
Member

Choose a reason for hiding this comment

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

Why an enum? I think you just need this matcher as static in the AerospikeClientInstrumentation.
And are there many other methods in this class? As a result, you have to list all relative methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AerospikeClient class has many methods,but it is possible to remove some methods, I'll remove them.

Copy link
Member

Choose a reason for hiding this comment

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

Please remove this enum. This is just a static list.

@wu-sheng
Copy link
Member

Any update?

@dd1k
Copy link
Contributor Author

dd1k commented Jun 29, 2023

yes,I update after get off work.

@dd1k
Copy link
Contributor Author

dd1k commented Jun 29, 2023

already updated

Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Once plugin tests pass, I am good.

@wu-sheng wu-sheng merged commit d42e245 into apache:main Jun 29, 2023
yangyulely pushed a commit to yangyulely/skywalking-java that referenced this pull request May 25, 2024
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.

2 participants